Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable multiple registrations #29

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dpanta94
Copy link
Member

@dpanta94 dpanta94 commented Dec 17, 2024

The primary funcitonality that is being introduced is allowing you to make multiple registrations just by using the API exposed.

Previously, because internally there were calls to wp_script_is or wp_style_is even if you removed the asset altogether you could not register more than once.

Registering more than once can have some advantages - listing a couple that pop on the top of my head.

  1. Adding a script which is extending another script related to block editor.

Now you can modify your original's script dependencies to include your new script. Making managing when your script should be included easy. Wherever my original script is included, also please include my extension.

  1. Enabling us to overwrite based on filters.

Also this PR introduces a magic method which enables us to call script or style methods from WP on the asset instance itself. You only need to call the method without the wp_ prefix and replacing with asset wherever the script or style would be.

So now you can do:

$asset->deregister_asset();

// will correctly call wp_deregister_script( $slug ) or wp_deregister_style( $slug ), based on the asset's type.
// The slug is always passed as the first param and you can pass anything else you might need.

See an example of a snippet we had to provide support to achieve the functionality being added here:
https://lw.slack.com/archives/C01SBD5T03V/p1732799298015619?thread_ts=1732270965.841129&cid=C01SBD5T03V

@dpanta94 dpanta94 requested review from lucatume and bordoni December 17, 2024 18:00
@dpanta94 dpanta94 self-assigned this Dec 17, 2024
@dpanta94 dpanta94 added bug Something isn't working enhancement New feature or request labels Dec 17, 2024
@dpanta94 dpanta94 marked this pull request as ready for review December 17, 2024 18:01
@dpanta94 dpanta94 requested a review from borkweb December 17, 2024 18:01
*
* @return static
*/
public function __call( $method, $args ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is smart and saves some lines of code, but I do not think makes for readable code.
I would unroll the __call method and explicitly create each method that is now part of the @method annotation.
This should likely not be a gateway to all wp_ functions, but to a limited, controlled and tested set of them.
What would this do if I call $asset->footer?
If you end up having to maintain a safe-list of functions that can be called, you're better served by an explicit API made of concrete methods.

@borkweb
Copy link
Member

borkweb commented Dec 19, 2024

This looks pretty great (once Luca's comment is addressed) 🕺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants