-
Notifications
You must be signed in to change notification settings - Fork 384
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
Add service for entity registrant detection #6682
Add service for entity registrant detection #6682
Conversation
Plugin builds for b9aea83 are ready 🛎️!
|
* @type array $shortcode List of default shortcode. | ||
* ] | ||
*/ | ||
const DEFAULT_ENTITIES = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of storing this here, I think it would be better to have a $initial_entities
variable which is null
to begin with, then when the callback wrapper is invoked it can check if $this->initial_entities === null
, and of so, populate the initial array with what is registered.
Nevertheless, I wonder if it is necessary? If we capture all of the registered entities at prepare
and then diff them with what is registered at finalize
, why do we need to also need the default entities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of storing this here, I think it would be better to have a $initial_entities variable which is null to begin with, then when the callback wrapper is invoked it can check if $this->initial_entities === null, and of so, populate the initial array with what is registered.
Currently, We are doing the same but with $registered_entities
. In prepare()
, we collect all registered entities and in finalize()
we update the same property with a difference of registered entities.
Nevertheless, I wonder if it is necessary? If we capture all of the registered entities at prepare and then diff them with what is registered at
finalize()
, why do we need to also need the default entities?
Since We are checking all the callbacks from all hooks. It will also track the WordPress default functions which registers default entities. which I think is unnecessary to track. (Since the idea to detect if any theme/plugin is registering entities or not) and will increase the REST API payload.
So to exclude WordPress default entities in finalize()
this const is being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I haven't been able to respond to this yet. I'll have to dig more deeply into the PR and its changes in order to be able to reply. That'll have to come next week.
src/EntityRegistrantDetection/EntityRegistrantDetectionManager.php
Outdated
Show resolved
Hide resolved
PHP Warning: hash_equals(): Expected known_string to be a string, null given in /srv/www/amp-local/public_html/wp-content/plugins/amp/src/EntityRegistrantDetection/EntityRegistrantDetectionManager.php on line 109 |
9ba9773
to
0e5aa72
Compare
I have addressed this in 0e5aa72 commit. |
Use injector instead of direct initilizing object Add dependancy in counstruct method instead of direct initilizing object
0e5aa72
to
20f4291
Compare
$result = call_user_func_array( | ||
$this->get_callback_function(), | ||
array_slice( $args, 0, (int) $this->callback['accepted_args'] ) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why the outdated and slow call_user_func_array()
is used here?
This could be rewritten to use a faster variadic arguments call:
$result = call_user_func_array( | |
$this->get_callback_function(), | |
array_slice( $args, 0, (int) $this->callback['accepted_args'] ) | |
); | |
$callback = $this->get_callback_function(); | |
$result = $callback( ... array_slice( $args, 0, (int) $this->callback['accepted_args'] )); |
Note that it cannot be done as a one-liner because of PHP 5.6 parsing.
*/ | ||
protected function wrapped_callback( $callback ) { | ||
|
||
return $this->injector->make( CallbackWrapper::class, compact( 'callback' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Injecting the injector should always be avoided whenever possible. There are two viable options for doing so here:
If CallbackWrapper
is considered a service or similar:
Instead of injecting the injector (and hence, giving control to the object of the entire rest of the application), it would be preferable to inject a CallbackWrapperFactory
instead which is specialized on directly instantiating instances of type CallbackWrapper
.
Using a factory is always a valid way of decoupling a new
call, as the injector can override the factory implementation if needed.
If CallbackWrapper
is considered a Value Object:
If we consider CallbackWrapper
to be a value object conceptually (which is a bit far-fetched but not entirely wrong), then it would mean we can freely new
that value object directly, instead of having it be instantiated via the injector.
Summary
Fixes #6516
This PR adds new services "EntityRegistrantDetection", which detect the function which registers new entities such as post type, taxonomy, shortcode or block. And provide that info via REST endpoint.
Sample REST response.
Checklist