wptt/admin-notices

Package for standardizing how themes output admin notices.

admin-notices's Issues

"Dismiss" button not persistently dismissing notices

I found I would dismiss notices, but on reloading the page they'd be back. The option wasn't being set in the site's options table.

Further investigation showed that the javascript console was seeing an error: dismissBtn was coming back null.

I changed the file src/Dismiss.php from this:

var dismissBtn  = document.querySelector( '#wptrt-notice-<?php echo esc_attr( $this->id ); ?> .notice-dismiss' );

to this:

var dismissBtn  = document.querySelector( '#wptrt-notice-<?php echo esc_attr( $this->id ); ?>' );

...and it worked. It appears that the class .notice-dismiss isn't being added.

I'm running WordPress 5.6.1.

Dismiss action

Anyone with an vanilla JS or code in general to point to for handling the dismiss action for an admin notice would be much appreciated.

allow full HTML in notices

It's cumbersome and unnecessary to restrict what HTML can be used inside notices. For example, it's common to want to add a form, or embed a video, or a million other things than what's allowed.
What's more, these notices are added by the developer, who has full access to the code and database. They have much more direct methods to hack a website than adding malicious JS.
I would suggest all that code relating to restricting what HTML tags can be used inside notices should be removed.

Deprecation Notice in PHP 8.2

( ! ) Deprecated: Creation of dynamic property WPTRT\AdminNotices\Notice::$title is deprecated in \wptrt\admin-notices\src\Notice.php on line 130

Bootstrapping, constructors, and hooks

This should perhaps be broken into a couple of different tickets, but I think it all ties together to some degree. It's also some thoughts on the bigger picture as we develop more packages in the future.

Generally speaking, the __construct() method should not call add_action() as done here: https://github.com/WPTRT/admin-notices/blob/master/src/Notices.php#L40-L47. The constructor's only job should be setting up object properties if needed.

Instead, action/filter hook calls should happen in a separate method that I'd name boot() like so:

public function boot() {
	// add_action() and add_filter() calls go here.
}

We'll dig into the "why" of this, which I hope will become clearer by the end. But, first, let's look at the bootstrapping code in its current state:

$notices = new \WPTRT\AdminNotices\Notices();

// Add a notice.
$notices->add( (string) $id, (string) $title, (string) $content, (array) $options );

I do worry that theme authors might just drop this anywhere in functions.php and have a global $notices variable with that (we might need to caution against that). But, it mostly looks good.

The issue for me is different. In a real-world project, I'd ideally register the Notices class as a single instance with a container like so:

// Register early.
$container->singleton( Notices::class );

// Bootstrap later when needed.
$container->resolve( Notices::class )->boot();

The problem here is that by using __construct() to run add_action() it's combining setting up the object and executing actions. There's no way I can wait to bootstrap the actions. And, child themes cannot simply remove the Notices instance from the container between the point of registration and bootstrapping.

I think bootstrapping would work better as:

// Create a new Notices instance.
$notices = new \WPTRT\AdminNotices\Notices();

// Add a notice.
$notices->add( (string) $id, (string) $title, (string) $content, (array) $options );

// Bootstrap notice actions.
$notices->boot();

---

On a "larger picture" note that goes beyond this ticket and beyond this particular TRT package, I'm not sure how I feel about combining both the "collection" of notices with the actions/filters. I've struggled with how to best simplify that and whether there should be an additional "component" or "manager" class. I feel like that might overcomplicate things for something that we're trying to keep simple for theme authors.

Basically, it comes down to separation of concerns. Notices is doing two jobs. Should it be concerned with anything other than storing Notice objects? And, this may not be a big concern for this project specifically. Just posing the question.

Anyway, something to think about.

Class / Style attributes stripped from allowed tags

Hi All,

Was trying out your package recently and noted that it stripped the class / style attributes from the em tag I was trying to use in the admin notice content. In my case this was to add some (font awesome) icons to the notice.

Is there any appetite to allow the class / style attributes on the allowed tags for this use case?

Original Project Code

I meant to push this live ages ago but forgot. I'm just opening this ticket to share the original code that I threw together. Feel free to use any bits and pieces or discard it altogether. There might be an idea or two worth looking at in there.

admin-notices.zip

Similar package

What a coincidence!
I've just started an admin notice class: https://github.com/szepeviktor/wordpress-persistent-admin-notices
(I know, it uses WordPress as a namespace)

Please add <br> tags to the allowed list.

Before:

private $allowed_html = [
		'p'      => [],
		'a'      => [
			'href'  => [],
			'title' => [],
			'rel'   => [],
		],
		'em'     => [],
		'strong' => [],
	];

After:

private $allowed_html = [
		'p'      => [],
                'br'      => [],
		'a'      => [
			'href'  => [],
			'title' => [],
			'rel'   => [],
                        'class'   => [],
		],
		'em'     => [],
		'strong' => [],
	];

readme autoload

This not Work

$loader->add( 'WPTRT\\AdminNotices\\Notice', get_theme_file_path( 'path/to/admin-notices/src' )

$loader->add( $prefix, $path );

$prefix - This should be the namespace of the project.

This is work and load the classes

$loader->add( 'WPTRT\\AdminNotices\\', get_theme_file_path( 'path/to/admin-notices/src' ) );

you have to change it to only The namespace

No longer works at all

See invalid PHP at:

admin-notices/src/Notice.php

Line 41 in d574d0c

private title;

I think the solution for most people is to use the previously working commit:

composer require wptrt/admin-notices:dev-master#42101e8cf841b17342fcef984a06caf63990c6f2 

but my projects which use this have multiple dependencies which use this so I'm still figuring out my own solution.