!!!TASK: Bootstrap instantiates request handlers internally #3371
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kitsunet
force-pushed
the
task/90-bootstrap-requesthandler
branch
from
de760a5
to
6c611a6
Compare
This cannot ever be green on CI as the necessary changes to the BuildEssentials (register/preselect testing request handlers) are a hard block in either direction, whichever comes first breaks it.So you would have to run tests locally or trust me and we fix anything that comes up after.... |
kitsunet
added a commit
to kitsunet/BuildEssentials
that referenced
this pull request
Jun 22, 2024
mhsdesign
reviewed
Jun 23, 2024
There was a problem hiding this comment.
thanks for this ;) We also stumbled upon this complexity for the neos/setup part ... this change does make things easier. Though we do have to live with the fact that the 6.0.0 tag of neos/setup will not be compatible with Flow 9.0 anymore which it is currently and tagged: https://github.com/neos/setup/blob/ecd193d814aea502aad71b0907f193de08fc9d0d/composer.json#L8 (im stupid sorry lol).
It seems unnecessarily complex to have code register a request handler as build instance. Instead it is now registered by class name. The Flow Bootstrap will then create instances while determining the best matching request handler. To facilitate this being lazy the RequestHandlerInterface::getPriority method is now declared static so that we can decide beforehand if the priority is higher than any previously found best match. Depending on possibly request handlers and specifically in all preselected cases this all reduces the amount of created objects and possibly the amount of canHandle calls. A microptimization but which affects all requests. Primary concern was to make registration easier and internalize instantiation. As small simplification the `Bootstrap->setPreselectedRequestHandlerClassName` will also register the given class name as possible request handler removing one step. This is breaking if you have your own request handler implementation. The `getPrioritty` method needs to declare an `int` return type and must be static. Additionally the registration happens via `Bootstrap->registerRequestHandlerClassName(YourRequestHandler::class)`.
kitsunet
force-pushed
the
task/90-bootstrap-requesthandler
branch
from
6c611a6
to
ae54ccd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It seems unnecessarily complex to have code register a request handler as build instance. Instead it is now registered by class name. The Flow Bootstrap will then create instances while determining the best matching request handler. To facilitate this being lazy the RequestHandlerInterface::getPriority method is now declared static so that we can decide beforehand if the priority is higher than any previously found best match.
Depending on possibly request handlers and specifically in all preselected cases this all reduces the amount of created objects and possibly the amount of canHandle calls. A microptimization but which affects all requests. Primary concern was to make registration easier and internalize instantiation.
As small simplification the
Bootstrap->setPreselectedRequestHandlerClassNamewill also register the given class name as possible request handler removing one step.This is breaking if you have your own request handler implementation. The
getPriorittymethod needs to declare anintreturn type and must be static. Additionally the registration happens viaBootstrap->registerRequestHandlerClassName(YourRequestHandler::class).Also the RequestHandler now needs a static constructor
RequestHandlerInterface::fromBootstrap(Bootstrap $bootstrap): selfthat the Bootstrap will call when instantiating one.If the RequestHandler for some reason was used as a bridge to the outside world and the instance carried more information besides the bootstrap, you can "sneak in" your instance with anything you want by setting it via
Bootstrap->setEarlyInstance(YourRequestHandler::class, $yourInstance)and this will be used instead of creating a new instance via the static constructor. You still need to register the class as possible request handler though.