g.oechsler should also receive credit for this patch.
Follow-up on #1798214: Upcast request arguments/attributes to full objects and #2055857: [policy, then patch] Standardize service names
In #1798214: Upcast request arguments/attributes to full objects we introduced RouteEnhancers as part of the DynamicRouter from Symfony2 CMF.
RouteEnhancers essentially are request manipulators. More precisely: When a request is processed through DynamicRouter::matchRequest()
all registered RouteEnhancers are invoked and get a chance to alter the request. Among other things, this gives us the chance to upcast variables from the URL.
Upcasting means that variables from a request, e.g. so called {slug}'s (for example /node/{node}), are converted to full objects (e.g. Node) which results in the actual controller (page callback) receiving the fully loaded objects instead of the plain variable string.
Specifically for that (upcasting) #1798214: Upcast request arguments/attributes to full objects implements a special RouteEnhancer called "ParamConverterManager" which allows specialized ParamConverters to be registered with it in order to further abstract route enhancing specifically for upcasting said {slug}'s into proper objects (and just that). Furthermore, the ParamConverterManager throws a 404 if upcasting for one or more of the given {slug}'s fails (e.g. if a non-existent node id was requested).
In my opinion the main difference to Drupal 7 is (despite the fact that the underlying system is completely different) that instead of specifically registering and invoking 'menu loaders' (e.g. node/%node => node_load()) the ParamConverterManager does not know which ParamConverter is responsible for upcasting which {slug}... It simply loops over all the registered converters and hopes that, when finished, everything has been loaded properly. This gives us more flexibility (and less complexity) but also less control over what exactly is happening.
The goal of this issue is to further improve the ParamConverterManager and the developer experience for ParamConverters.
Comment | File | Size | Author |
---|---|---|---|
#120 | 1943846-120.patch | 40.08 KB | fubhy |
#114 | 1943846-113.patch | 39.76 KB | fubhy |
#114 | interdiff.txt | 1.26 KB | fubhy |
#110 | 1943846-110.patch | 39.76 KB | fubhy |
#110 | interdiff.txt | 5.73 KB | fubhy |
Comments
Comment #1
fubhy CreditAttribution: fubhy commentedAttaching @g.oechslers latest patch from the original issue #1798214: Upcast request arguments/attributes to full objects.
Comment #3
fubhy CreditAttribution: fubhy commentedCurrently, the ParamConverterInterface::process() method works by accepting two array arguments by reference. The first one is the original $defaults array as seen in RouteEnhancerInterface::enhance() - This is where we replace the original string value with the fully loaded (upcast) object. The second one is some sort of 'info array' in which we keep track of the names of the variables that have already been loaded.
One of the goals of the ParamConverters should be "Developer Experience". And those arrays really aren't what I'd call good DX. We should try to move the logic of keeping track of already converted {slug}'s. Furthermore, IMHO ParamConverters should not be allowed to directly alter the $defaults array but instead only alter an array that we construct in ParamConverterManager that only contains the variables OR not hand any arrays by reference at all and instead rely on @return (ParamConverter returns an array of variables that it touched which is then merged into $defaults by ParamConverterManager). This would also allow us to keep track of the already converted variables.
Comment #4
fubhy CreditAttribution: fubhy commentedTagging and bumping to major (original issue was set to major before).
Comment #5
fubhy CreditAttribution: fubhy commentedRe-roll, code style fixes and also a first stab at trying to simplify how single param converters operate with $defaults and $converted.
Comment #6
podarokFYI http://drupal.org/documentation/git/interdiff
Comment #7
fubhy CreditAttribution: fubhy commentedGiven that #1906810: Require type hints for automatic entity upcasting is even easier then I thought it would be and properly solves upcasting in a much nicer way (explicit slug metadata definition in the route options, no guessing, etc. while still not requiring every entity type to register a specialized param converter as with the original patch in #1798214: Upcast request arguments/attributes to full objects) I am not 100% sure we actually want to continue here. I had a long chat with @sdboyer and @eclipsegc today about Blocks, Layouts, Displays, Contexts, Conditions, Upcasting, yadda yadda and we concluded that there is no way that SCOTCH is going to work without that anyways. At least we didn't see how.
So I'd kindly ask for reviews for #1906810: Require type hints for automatic entity upcasting too :).
Comment #8
Crell CreditAttribution: Crell commentedNo. We deliberately changed it from defaults to variables because we wanted to decouple this system from Route enhancers. $defaults makes no sense in any other context. (It barely makes sense in that context, frankly.) Leave it at $variables.
That said, accepting a variable and then returning rather than passing by reference I fully support.
I'm also wondering why we need the converted tracking. Why can't we just say "once it's an object, other converters are required to ignore it, period"?
Otherwise this looks reasonable. I would like to move forward with this patch regardless of typed data upcasting, although I agree with getting reviews on that one, too.
Comment #9
tim.plunkettWell, the Views UI converter can only operate if the Entity converter has run. So it skips that check, unlike the Entity converter.
The current patch doesn't change that, I'm just ensuring that we don't break it by enforcing what @Crell suggested.
Comment #10
fubhy CreditAttribution: fubhy commentedOkay, i'd prefer naming it $variables too. However, if we name it $variables we have to make sure that it only contains variables. There is more in $defaults than just variables.
I was a bit confused about that myself to be honest. However, if we do that, things like that ViewsUI converter would have to be implemented as enhancer.
Comment #11
Crell CreditAttribution: Crell commentedViews UI has its own converter? This is news to me...
How are values in an array not variables? :-) They may be objects, but they're always variables.
Comment #12
fubhy CreditAttribution: fubhy commentedYep, I guess it's okay for us to assume that param converters only run on not-yet-converted variables. That also would allow us to get rid of $converted and run ->process() for one variable at a time skipping those that have been converted already. That way we could also throw the 404 directly once a conversion to NULL occurs. To me that approach sounds much better anyways because it does not leave the responsibility of managing that array in the hands of the converter (converters really shouldn't have to worry about that, otherwise they are just another set of plain enhancers).
But still, I am more and more convinced (about 99% sure at this point) that we simply shouldn't and probably simply can't do upcasting with this magic/guessing anyways. And if it wasn't for the guessing we would still have this situation where we invoke every single param converter that is registered with the DIC. Everything that is not an entity needs a custom converter which then is invoked on every single page request.
Furthermore, I can't see how SCOTCH would ever be able to properly work with this. Maybe it's just beyond me but it seems impossible to solve Panels in a generic fashion without that metadata on the route. So the metadata does not only gets us to a place where we do not need to do all that guessing, it also gives us integration with other things (like Panels): TypedData is more than just a stupid object loader in this regard. It gives us information about the route and the contexts that will be available during the request (Page Manager!).
Yep. It's basically a second-layer of param conversion. It takes the previously upcast entity and wraps it in a ViewUI object. It relies on multiple levels of upcasting as a feature.
With "variables" I mean {slugs}. To me, ParamConverters (if we find a way for them to actually work for us, again... Sorry, but I really don't see how they ever will :-( ) are plain {slug} converters and $defaults is more than just the {slugs}. So if we call that method argument $variables then that's what it should be. A list of {slugs} and their current values and not a plain copy of $defaults.
Comment #13
Crell CreditAttribution: Crell commentedfubhy_: Uh. I think you're horribly confused. :-) There is *no difference* between what Symfony calls $defaults and what our ParamConverters call $variables except that $defaults doesn't make sense as a name except in the route property context. The distinction you're drawing between "slug converters' and "defaults" is nonexistent. Symfony doesn't differentiate between values that come from placeholders and from the route defaults array. They're all the same by the time they get to a route enhancer.
Comment #14
fubhy CreditAttribution: fubhy commentedCorrect, but I thought the goal of ParamConverters was to ONLY work on slugs. And there is more than the slugs in $defaults.
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedUploading my patch from #1798214-135: Upcast request arguments/attributes to full objects. I think it addresses much of what's being discussed here.
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedI want to particularly call out this part. What it does is move the magic defaulting of (entity) type from slug name from EntityConverter to ParamConverterManager. I actually wrote that with #1906810: Require type hints for automatic entity upcasting initially in mind, though reading recent comments on that issue, it might be the case that simply defaulting a 'type' might not be enough. I need to delve into that issue a bit more to understand what other metadata defaults would be needed.
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedComment #19
effulgentsia CreditAttribution: effulgentsia commentedWith ViewUIConverter converted.
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedinterdiff from 15 to 19.
Comment #22
Crell CreditAttribution: Crell commentedPassing the original array in would allow for things like upcasting one value based on another no? (I recall someone asking about that; I think Klausi?)
From a read through this seems sensible, but there may be details I'm missing. :-)
Comment #23
fubhy CreditAttribution: fubhy commentedThat was me I think. I still think we need to be able to do that (however often we will end up actually doing that). I also opened an issue for that back then (#1837388: Provide a ParamConverter than can upcast any entity.) but am hoping that we could solve it with #1906810: Require type hints for automatic entity upcasting by handing previously upcast arguments into the typed data constraints of a subsequent argument.
In any case interdependent upcasting has to be handled carefully as it greatly affects route matching. Matching routes against multiple sequential unknowns can become quite rough. So we should still encourage people to rather provide multiple routes when possible.
I'd still love to get some movement in and around that TypedData upcasting issue.
Comment #24
tim.plunkettThis looks infinitely preferable to #1906810: Require type hints for automatic entity upcasting.
Rerolled for the Views UI move, and added an isset (not all placeholders need upcasting)
Comment #26
fubhy CreditAttribution: fubhy commentedAssigning back to me after a good chat with @tim and @Crell.
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedThanks guys! Sorry for dropping the ball on this after #19. Very happy that you picked it up.
Comment #28
fubhy CreditAttribution: fubhy commentedOkay, the attached patch follows the pattern that we also use for access checks. The parameter converters are registered with the routes based on simple applies() checks during the route alter event. The converters are registered on a per-variable basis which reduces the complexity of the converters because they just have to worry about one variable at a time with this.
Comment #30
fubhy CreditAttribution: fubhy commentedStupid last-minute changes...
Comment #32
fubhy CreditAttribution: fubhy commentedForgot one views placeholder.
Comment #34
tim.plunkettI can't speak to the internal changes much, but the new EntityConverter code is very easy to understand. I like the idea of mimicking AccessChecks.
This is all Views specific stuff, so I'm not surre why we don't check for the interface anymore.
This is cool, and makes sense
Comment #35
fubhy CreditAttribution: fubhy commentedYeah, not sure why I removed that. I wrote those from scratch and then copy&pasted the old stuff over. Probably got lost on the way. We probably want to go 404 if $view is not available at that point.
Comment #36
yched CreditAttribution: yched commentedProbably not for this issue, but could that lead us to a point where individual routes could specify custom, ad-hoc converters for their params ? Like, this line could support existing 'parameter_converters' specified in the route definition rather than just ovewriting the option ?
From what @tim.plunkett explained to me on IRC, the current situation is that custom converters are discouraged because we currently load *all* existing converters on all requests.
Not sure having the same method name than ParamConverterInterface::applies() is a good idea. The methods are related, and the verb works well in the case of ParamConverterInterface, but not really on the manager. Maybe something like getApplicableConverters() ?
Misleading var name, those are converters keyed by parameter name, not parameters themselves.
If I get the code flow right, it seems the converters will always be loaded in ParamConverterManager::applies() ?
I guess the phpdoc is copy/pasted from access services and needs to be adjusted ? ;-)
Comment #37
tim.plunkettAccessCheckInterface::applies() are called by AccessManager::applies(), so that pattern follows.
Comment #38
fubhy CreditAttribution: fubhy commentedAre we talking about closures here? Interesting idea, but out of scope for this issue indeed. Not sure if we want closures for this though as that would bring us back to a place where the final value would be totally unpredictable.
Yeah but that only runs on route cache rebuild.
Indeed :)
I am starting to wonder if it is maybe a bit weird that we allow multiple converters to be applied to the same route. To me that looks like there is too much potential for failure and too many unknowns in the mix that each converter has to take care of. This is not like explicit plugin definition decorators that can be wrapped easily as they all implement the same interface and can be infinitely nested.
What if we only allowed one converter for each argument? Converters are services anyways and could be injected into other converters... Like, in case of the ViewUIConverter we could inject the EntityConverter to do the initial entity upcasting but then further 'decorate' it into a ViewUI object. We already have weights/priorities for converters so we could simply make it so that the first converter for which applies() evaluates to TRUE gets registered, not all of them.
We cannot directly compare this to access checks where it indeed DOES make sense to have multiple layers of access checks. But since those don't mess around with the variables it is not a problem. In this case, however, all the converters more or less rely on each other. We already have an example of that with the ViewUIConverter which currently does
Yeah, okay... But what if the variable is not a View yet at that point? Do we want to throw a 404? Maybe the route controller expects a ViewUI object and even typehints on that which would throw an exception if we seamlessly bail out here?
I believe we either have to find a concept for converters to safely depend on each other or we only allow a single converter as proposed before (after all those are services).
Setting back to 'needs review' to collect some more opinions.
Comment #39
yched CreditAttribution: yched commentedNot sure how closures relate to this ? My point was to have a way to specify "for *this* param of *this* route, use MyVerySpecificConverter, but don't bother loading the class for any other route that doesn't explicitly specify it". Something like:
Converters being currently loaded on all routes de facto means custom ad-hoc converters are discouraged, since loading them all would be a performance drag when contrib goes wild. In that sense, adding a custom converter is being a "bad citizen", since it impacts everyone outside of my specific case, and things would be a mess if everyone did the same. "Extensible by being a bad citizen" doesn't really match "extensible" :-)
This in turn forces us to come up with a limited set of generic converters that need to fit 99% cases (and are tried successively on each argument until one works), since adding a custom one is a "heavy" choice. This in turn means that we don't really control our URLs (as #1946404: Convert forms in field_ui.admin.inc to the new form interface showed, where admin/structure/types/manage/article/fields/field_foo had to be turned to admin/structure/types/manage/article/fields/node.article.field_foo).
Sorry if this adds noise here - then again it seems kind of related to your "one converter per argument" line of thought above.
Comment #40
fubhy CreditAttribution: fubhy commentedOh, absolutely... That's one of the goals for this issue and inspired by ViewUIConverter which bugged me all the time because it runs on every route as of now. Hence I tried to apply the same pattern used by AccessCheckManager here. I think I misunderstood your previous comment. I thought you wanted to go even further and also allow closures in addition to services for "ad-hoc" conversion of specific routes only.
We should still try to provide a set of generic route converters that solve 99% of the user-cases. However, at the same time, we should encourage custom converters to be written whenever required. With this patch that becomes possible as, with the Route alter event approach (registering the converters on cache rebuild rather than on run-time), we do not add any weight the run-time evaluation of unrelated routes.
No noise, absolutely valid points.
Comment #41
fubhy CreditAttribution: fubhy commentedNot sure how the others feel about it but that would be really close to what I had in mind when proposing to only allow a single converter per argument (I still think that nesting those might get us into more trouble then it would benefit us).
Explicitly specify the converter (e.g. 'entity' === 'paramconverter.entity') and thereby make the 'applies()' phase redundant (we already know which converter applies because it's service id has been explicitly declared).
Comment #42
fubhy CreditAttribution: fubhy commentedThis patch only allows one converter per parameter and that converter is explicitly declared in the route definition. This requires knowledge of the paramconverter service id so not sure about DX here but technically this feels more stable.
Comment #44
effulgentsia CreditAttribution: effulgentsia commentedWhy is it technically more stable? I think the DX is worse, and that the earlier patch, where a converter applies based on which keys are used (e.g., 'paramconverter.entity' is used if the parameter uses an 'entity_type' option) was cleaner.
I haven't thought about the pros/cons of this, but even if we want it, can't we achieve it via priorities? So, of all the converters that return TRUE for applies(), use the highest priority one?
Comment #45
effulgentsia CreditAttribution: effulgentsia commentedComment #46
fubhy CreditAttribution: fubhy commentedYes, that's also possible I guess. The problem with multiple converters is that they all build and rely on each other in some way or another. And they do so without any interface or other form of contract that would provide some sort of reliability for the values that they are working with. For example, the ViewUIConverter is completely useless if it does not get a View entity. And if it wasn't for the interface check that it has now it would even break. The problem really is that multiple converters lead to iterative overriding of the original value without sharing any information between each other.
Also, the route arguments and the route controller do have some kind of contract so the relation between them is not even nearly as dynamic as the interaction between the route + access checks. You can always load more access checks on a route but the arguments and their types are bound to the route controller and especially the type hinting that is (potentially) going on there.
Comment #47
fubhy CreditAttribution: fubhy commentedWhat I am saying is that we can not think of param converters as things that we are going to load onto the routes dynamically and it'a also unlikely that we are going to replace the converters for a route. That's simply not a use case unless you replace the controller at the same time. I rather see controllers as a neat addition to reduce repetitive code for loading things (especially entities) and throw 404's when the 'thing' that we are trying to load is not available.
Comment #48
yched CreditAttribution: yched commentedAgreed that
is worse DX, but only if specifying a converter service ID is turned into a requirement for all routes/parameters - but that doesn't have to be the case ?
We'd want to *allow* specific routes to specify a custom converter, most likely an ad-hoc converter built specifically for the route (or set of routes), in which case knowing the service id of the converter you just custom coded is not a real drag.
This doesn't prevent us from shipping "default converters that cover the 80% use cases" like direct value passing or the current EntityConverter, that would still fire by default as they currently do, when no explicit converter is specified for the argument ?
Comment #49
msonnabaum CreditAttribution: msonnabaum commentedI like #34, although the stacking is awkward and feels like the responsibility of a converter.
I agree with #48 that you should be able to specify a custom callback. It could follow the same conventions as _controller.
I also don't see a use case for more than one key on a parameter. Having a simple 1:1 mapping like parameters: {view: 'my.custom.converter'} makes sense to me.
Comment #50
Crell CreditAttribution: Crell commentedThis would be the first time we're coupling routes to the DIC, which makes me very uneasy. I'm almost leaning toward #32's approach there instead, but not firmly decided. Other than that, and the verbosity of the route definition, #42 is actually looking pretty good.
Comment #51
fubhy CreditAttribution: fubhy commentedPicking this up again.
So I assume that, by now, we all agree that "explicit declaration" is the way to go... I understand that services might seem a bit heavy for upcasting. Also, they would require knowledge about the service id's which, again, sucks DX-wise. So, how did we feel about #32? I still want to make sure that we only have ONE param converter per argument though....
To summarize:
1. We want explicit declaration (no guessing)
2. We don't want to explicitly tie route declarations to services so specifying the converter service id is a no-no.
3. We only want ONE converter per route. The concept used by the access manager (multiple access checks can be registered for a route) does not make sense here because we need some degree of predictability for what we get from a {parameter}. Please correct me if I am wrong here... In case of access checks, the order in which they are applied generally does not matter. However, in case of upcasting it is crucial.
The views case (tempstore for edited views) does not justify a multi-layer upcasting system imho.
Comment #52
effulgentsia CreditAttribution: effulgentsia commentedI don't know whether #51.3 will end up being too limiting or not, but I think it's fine to start off that way, so +1 to #51.
Comment #53
Crell CreditAttribution: Crell commentedAgreed on #51.
Comment #54
fubhy CreditAttribution: fubhy commentedLet's try this... I like the syntax. It's also closer to the access checks. Since this patch uses a 'first come, first serve' approach we might need to implement weighting for the converter services.
It's possible that I missed some routes that got added/converted in core recently.
Comment #55
Crell CreditAttribution: Crell commented"Converts a parameter to its corresponding object."
Am I reading this correctly that we'd still allow for "automatic" conversion, since a converter can decide if it should apply if the route itself hasn't claimed one? I'm a bit confused here.
Edit: Wait, I think I get it. You can specify "convert with this service ID" with _converters, OR specify "this should be object type X" with _parameters, and the latter will get turned into the former when building routes. Do I have that right?
Comment #57
fubhy CreditAttribution: fubhy commentedYes, you can manually set a service id in the route definition if you wish.
Comment #58
fubhy CreditAttribution: fubhy commented#54: 1943846-54.patch queued for re-testing.
Comment #59
Crell CreditAttribution: Crell commentedOK, that makes sense to me. We just need to document it well, and note that using _parameters is the preferred mechanism.
Comment #61
fubhy CreditAttribution: fubhy commentedOkay, those fails are all related to routes that I missed (basically because they got added after #32 and I did not look for new routes in the most recent patch). Anyways... Before I re-roll with those routes taken into consideration: Do we agree with the last patch (conceptually)?
Comment #62
effulgentsia CreditAttribution: effulgentsia commentedOverall, +1, but how about...
1. Do we need 'parameters' to have a leading "_"? The leading underscores are needed in subkeys of 'defaults' and 'requirements', since those can also contain URL parameter names. But within 'options', I'm not clear what the purpose of the "_" is.
2. I suggest one more level of nesting after the parameter name. Like this:
Or:
Because...
This can then be:
And then _converters wouldn't need to be a separate option, but could be part of each parameter's definition. For example:
Since $options will likely be needed by all converters, let's pass it as a parameter to convert(), after $value. The rest: $defaults, $request, and $route should rarely be needed. In fact, let's make it $parameter_options (i.e., the manager passes it $options[$parameter]).
Comment #63
fubhy CreditAttribution: fubhy commentedSure, we can switch back to not using the underscore. I just did that to make it look the same in the .yml files ;).
Pretty much agree with the rest too but I thought people would prefer a simpler syntax. Am happy to go with another level of nesting for setting the options properly. Also, setting the converter as part of the options array makes sense, good point.
However, I disagree with this (or don't understand it).
As mentioned before, I am pretty sure we DON'T want to allow multiple layers of param converters stacked on top of each other. Symfony doesn't do that either by the way.
@tim.plunkett added some code comments to ViewUiConverter when he first wrote it which suggested that he tried to implement it in a generic fashion where the argument of the option (e.g. 'tempstore': 'view') would define the collection from which the object should be loaded. However, that upcaster also (non-generically) wraps the existing entity object in a ViewUI object if there is nothing in the tempstore which makes it non-generic anyways (which is fine). So, simply going with something like "type: 'tempstore.views'" should be sufficient here.
Comment #64
effulgentsia CreditAttribution: effulgentsia commentedI didn't mean to imply that multiple option keys means multiple converters. I'm assuming in that example that my_custom_module.views_converter has a high priority and therefore wins. But that perhaps that converter might also be interested in those other keys (maybe the custom converter is flexible enough to be reusable for other entity types and tempstores).
Comment #65
effulgentsia CreditAttribution: effulgentsia commentedAre objects loaded from tempstore collections always of a different class than entities loaded from permanent storage? If so, then I'm okay with
type: "entity.view"
andtype: "tempstore.views"
syntax, since they are in fact different types. If the objects can be of the same class though, and only differ by storage location, then I'd want to be cautious about overloading type to mean more than just the type.Comment #66
fubhy CreditAttribution: fubhy commentedAh, right.. Yeah, that generally makes sense. But in this case we don't really need that as the converter used for Views UI only works for Views and can't be turned into a generic one anyways (@see next paragraph).
No, they are not, but in case of Views they wrap the View object in a ViewUI object. No idea how that is used but that's what they do. So at least in case of views we can't use a generic tempstore param converter.
Anyways, let's not turn this into a Views UI Converter discussion.
Will post an updated patch including the missing routes asap.
Comment #67
tim.plunkettComment #68
effulgentsia CreditAttribution: effulgentsia commentedOne more request on this. Can we change
type: 'entity.aggregator_feed'
totype: 'entity:aggregator_feed'
(i.e., just change the "." to a ":")? The reason is that I still would like to leave open the possibility that we do #1906810: Require type hints for automatic entity upcasting after July 1, which we can do as long as it's not API breaking. And ":" is the standard plugin derivative delimiter, so using it now gives us the ability to later swap in a TypedData based converter without breaking any APIs.Comment #69
effulgentsia CreditAttribution: effulgentsia commentedNot sure how that tag got removed.
Comment #70
fubhy CreditAttribution: fubhy commentedI just reopened #1906810: Require type hints for automatic entity upcasting and posted a comment and updated the issue summary with what has been swirling around my head for some time now.
Comment #71
fubhy CreditAttribution: fubhy commentedAssigning this back to me now that we have a plan. First step: Improve DX with param converter. The actual syntax of the route definitions won't matter much so we are going with what we had. This is really about not breaking routes while improving the ParamConverterManager mostly.
Conversion to typed data upcasting is afterwards happening in #1906810: Require type hints for automatic entity upcasting which will be a param converter.
Comment #72
Dries CreditAttribution: Dries commentedI like the new, simplified direction that we're taking with this. The syntax seems a lot easier. Looking forward to a patch; hopefully it comes pretty soon so we can still get it in.
Comment #73
fubhy CreditAttribution: fubhy commentedOkay, looking for conceptual review here...
This patch introduces a concept that involves multiple layers for solving the problem in a way that allows easy overrides on any level and still gives us the ability to manually define the parameter definitions in the routing.yml wherever required.
I slightly changed the strategy I proposed in #1906810: Require type hints for automatic entity upcasting in a way that makes it more flexible as it splits up the responsibilities further than I originally planned. We now have these layers:
TypedDataResolverManager (automatically deriving typed data types)
Invoked by a RoutingEvents::ALTER subscriber (TypedDataSubscriber). Iterates over every registered typed data resolver service (TypedDataResolverInterface) to try and generate typed data definitions based on whatever strategy that resolver service implements. So far we got two resolver strategies: a) EntityResolver (_entity_form/_entity_list) and b) Reflection (match the type hint of a controller argument to a typed data type).
ParamConverterManager (registering converters)
Invoked by a RoutingEvents::ALTER subscriber (ParamConverterSubscriber) with lower priority than that of TypedDataSubscriber so it runs after we run through the different typed data type resolver strategies. Iterates over every parameter defined in
$route->getOption('parameters')
(basically, the array structures our typed data resolvers previously filled with stuff and all the parameters that were defined manually). And tries to find a converter service for each of them. There is a maximum of one converter service for each parameter as multiple layers of parameter converters for the same parameter don't make much sense imho. If it finds one that applies, it writes the it's id in the route options ($parameter['converter'] = $id_of_service_that_applies
) and continues with the next parameter (if any).RouteEnhancer (actual upcasting)
The ParamConverterManager is also a RouteEnhancer (as it was before too). It then iterates over all defined parameters and invokes the previously set converter to try and upcast the given argument.
Note: The reflection typed data type resolver is pretty useless at the moment as we do not have a way to properly (reliably) map a type hint on a specific controller (of which we don't have many anyways) to a typed data type because the interface might be ambiguous. @see #2028097: Map data types to interfaces to make typed data discoverable
I am not going to run this through testbot for the time being as I did not write any unit tests for this so far anyways and I did not touch existing route definitions which will be necessary in a few cases (shouldn't be that many places though). Also, I did not touch the views ui converter yet!
Comment #74
fubhy CreditAttribution: fubhy commentedFixed some copy&paste screwups and confusing docblocks.
Comment #75
fubhy CreditAttribution: fubhy commentedAdding validation and cleaning up a little bit after reviewing it myself ;)
Let's just run testbot anyhow...
Comment #77
BerdirThe type also includes the bundle if it's known, e.g. when calling getType() on a loaded entity.
validate returns an object that contains the number of validations, see EntityValidionTest
Comment #78
fubhy CreditAttribution: fubhy commentedCheers @berdir.
Comment #80
fubhy CreditAttribution: fubhy commented@berdir just explained that validation does not work the way I thought it did. Sad panda :(. But we can live without it too I guess.
Comment #81
fubhy CreditAttribution: fubhy commentedAdding a few unit tests for the typed data type resolvers and fixing some minor things.
Comment #82
fubhy CreditAttribution: fubhy commentedRemoving accidentally added duplicate of the TypedDataConverter
Comment #84
fubhy CreditAttribution: fubhy commentedThat's way less fails then I expected. Especially if you exclude the Views fails which are all related to the not-yet-converted ViewUIConverter. Other than that it seems there are really just a few routes we have to manually define the parameters for.
Setting back to NR until this gets a good architectural review ;)
Comment #85
dawehnerThe sorting is now done in the actual manager, so this is a just a cleanup.
Follow up: This seems to be a really common pattern so somehow this could be moved to a common baseclass on the longrun.
Follow up: Throw exceptions in the entity manager instead.
It is a bit hard to understand why we need both param converter subscriber and type data resolver subscriber, because the param converter should know of these typed data resolvers? The problem is simply that it is basically impossible for a user to understand what is going on.
Simply use {@inheritdoc}
As far as I leared it, the rule is to put the most changing variable to the front, which is probably request or defaults, and then the other ones. Or in other words: the signature tells to convert variables, so these variables are stored in defaults/request so let's start with them. It just makes it easier to understand.
Similar argumentation as before.
Let's use $routes->all() instead of the magic.
Is there any reason for this ?: array() operator? I don't see why tbh. It makes it damn hard to read.
Can we describe what NULL means? (it could not been upcasted)
So this is just a similar pattern what the access manager does at all. Lazy load stuff and throw an exception as early as possible.
This probably should live in the typed data directory.
Could definition be something else then an array?
And if nothing exists throw maybe an exception?
My personal feeling would be that this logic is connected with the typed data manager itself, so should live there.
It feels like this line is potentially a good way to break stuff. It just returns and does nothing. Can't we set the type on the route and then let the converter fail? This feels also wrong, but maybe provides better and faster developer feedback.
Do we need this at all? $callable will never be an object or a closure according to self::resolveParameterTypes
Is there an explicit need to check that? It seems to be that this is already covered by the rest of the code below, but it is maybe useful to optimize this because of the performance.
Can we explain a bit better the usecases for the resolver resolver manager? (not only upcasting but also being able to store the information so things like princess can use it).
Do we need both injection via constructor and setter?
This full method has a lot of the logic of TypedDataConverter, is there a need for that?
Is there any usecase where you want to have a typed data on a route without being available via being loadable?
There is an explicit test: ViewUIObjectTest, so please add a test coverage as well.
I like to use @see, so you can directly access it on api.drupal.org
Let's also provide a route without _entity_form or _entity_type but with a node ...
I don't get why you have to mock the typed data manager, as it is just a simple constructor argument.
Same as before.
It would make sense to setup the manager at least, in a setup method.
Can you explain why we need this here?
Comment #86
fubhy CreditAttribution: fubhy commentedThe parameter converter is completely unaware of the type resolver layer. And that's how it should be. They are both entirely decoupled, which is good imho. You either manually define parameter definitions, or you rely on them being automatically resolved. The parameter converter doesn't care. It just reads them and then invokes the appropriate converter.
I agree, but we are not altering anything in the converters. We just return values. And the most important argument for that is $definition, followed by $name (which argument from $defaults needs conversion?) and, well, $defaults.
The $request is just an additional argument for good measure. But we don't alter anything or write to anything in the ParamConverterInterface instances at all.
Same with the applies() method. The $definition is really the most important argument there.
Agreed.
No. No idea why I wrote it like that. Probably copy&pasted from somewhere. It makes no sense really ;).
Yes, the ParamConverterManager definitely needs better documentation anyways.
Agreed.
Yes, it can either be an array:
Or a string:
This is just the resolver and it acts basically the same way as the EntityEnhancer that resolves the proper controller. In this case, though, if there is no parameter that needs to be converted (e.g. if we have _entity_form: node.edit but no {node} argument, then there is no reason to write anything into the route options).
It does live there, but we don't want to throw an exception. If the type is loadable, we want to load it, otherwise we just want to instantiate it through TypedDataManager::createInstance().
Yes, I guess that makes sense.
Yes, that's just there for performance reasons.
No, good point. I will adapt that in core.services.yml to use the setter instead.
Yes, because it's a separate typed data API. The typed data converter just "uses" the load method. It might be used elsewhere too. Let's keep that decoupled.
Yes, you might want to pass a DateTime object to your controller which is simply constructed from the arguments of a route (e.g. "/foo/{day}/{month}/{year}"). That does not need loading, just instantiation.
Good point.
I have to mock the typed data manager. I might not have to use reflection to write the typed data manager into the reflection resolver though. I can probably just pass it. Correct.
I am testing the sorting of the resolver objects in the resolver manager by checking
is_subclass_of()
with the pre-defined (expected) list of class names. @see the data provider for that test.Comment #87
fubhy CreditAttribution: fubhy commentedFixing the stuff mentioned in #85 and #84
Comment #89
fubhy CreditAttribution: fubhy commentedAdding a temporary solution for matching interface type hints to typed data types. This should be removed/changed once each typed data type explicitly declares it's interface (and thereby ensures that it is not ambiguous in typed-data land).
Should resolve most of the non-ViewUI-related test fails as most of the other fails are related to the fact that the previous reflection resolver code did not account for interface type hints.
Comment #91
fubhy CreditAttribution: fubhy commentedAdding reflection for _form controllers. Was not taken care of previously as we do not manually specify the buildForm() method in _form which caused it to get skipped.
Comment #93
fubhy CreditAttribution: fubhy commentedFixing a bug and starting to transform ViewUIConverter.
Comment #94
fubhy CreditAttribution: fubhy commentedFixing views_ui.routing.yml... Not sure if this is the way to go though.
Enough patches for today. Let's see if this comes back green.
Comment #96
alexpottDiscussed this with @EclipseGC and @Crell...
I don't think we should be doing magic guessing in the resolvers here... doing this will reduce risk and complexity. So for example, the user_role_edit route should look something like this:
The point being that when the developer writes a route they exactly what their slugs should be upcast to... Drupal guessing for them is complex and risky.
Comment #97
Crell CreditAttribution: Crell commentedBelated partial review of #91:
Typed Data Manager doesn't seem like it has a reason to be container aware...
Side note: As long as we're here we should fix this German idiom. :-) It's not valid English.
Document here what a null return means.
I know we're doing it in one or two other places, but this is a really crappy way of doing converter IDs. If we can do something else, we should.
I disagree with Daniel on this. :-) $routes is iteratable for a reason. It's totally reasonable to iterate a collection.
Comment #98
Crell CreditAttribution: Crell commentedComment #99
fubhy CreditAttribution: fubhy commentedOkay, talked this through with @Crell and @EclipseGc. We agreed to split this up into separate issues.
One for re-factoring ParamConverterManager and replacing the EntityConverter with a TypedDataConverter including explicit declarations for all paramters in routing.yml for now (manually).
The concept of TypedDataResolver will move to a separate issue: #2041907: Use some arcane magic to automatically discover typed data definitions for route parameters.
Furthermore, I am closing the separate issue for implement a TypedDataConverter as we will do that in here directly as there is no point in rewriting the EntityConverter first to then fully replace it with the new TypedDataConverter.
Will close this issue as a duplicate: #1906810: Require type hints for automatic entity upcasting
Comment #100
fubhy CreditAttribution: fubhy commentedJust re-uploading the same patch without the resolver layer. Will fail, obviously, due to the missing parameter declarations in all the routing.yml that we agreed to do manually for now.
Did not fix the comments from #97 yet either.
Will make this green tomorrow by putting the routing.yml definitions in order.
Comment #100. What a good comment count for a turning point.
Comment #102
fubhy CreditAttribution: fubhy commentedOkay, I think I found all routing.yml entries that needed parameters to be declared. Let's see...
Still need to rewrite the param converter tests as unit tests and improve the ParamConverter documentation.
Do you guys want a bare-bone patch without the routing.yml changes for easier review?
Comment #104
fubhy CreditAttribution: fubhy commentedOf course... Missed a test entity :/
Comment #105
fubhy CreditAttribution: fubhy commentedStarting to write some tests.
Comment #106
effulgentsia CreditAttribution: effulgentsia commentedThe overall architecture in #105 looks good to me. However, to get the DX right, I suggest splitting this into 3 parts:
- Part 1, this issue: Just the change to ParamConverterManager and ParamConverterInterface, but leave EntityConverter and ViewUIConverter in tact, only changing them as needed to conform to the updated interface.
- Part 2, #1983100: Provide a LoadableInterface for Typed Data objects.
- Part 3, replacing EntityConverter and ViewUIConverter with TypedDataConverter. Perhaps reopening #1906810: Require type hints for automatic entity upcasting and doing it there.
What do you think? Otherwise, my concern is that there's too many intricate pieces being changed here, and not enough analysis on whether each piece is truly necessary or what the DX impact of it is. For example, whether LoadableInterface is really necessary, and whether a 'class' override in the routing.yml file is the most logical way of loading from the tempstore.
Comment #107
fubhy CreditAttribution: fubhy commentedOkay, here is a patch that keeps EntityConverter and ViewUIConverter in place (and just refactors them to work with the new interface).
I updated the LoadableInterface patch over at #1983100: Provide a LoadableInterface for Typed Data objects (basically extracted everything related to loadable interface from this patch and then re-uploaded there).
Does not reduce the size of this patch that much though.
So, okay, let's follow Alex's plan from #106.
Comment #108
effulgentsia CreditAttribution: effulgentsia commentedThanks for being willing to split this up. I'm still in favor of getting the whole set of issues in, including #2041907: Use some arcane magic to automatically discover typed data definitions for route parameters, but this way, each one can get properly detailed and focused reviews.
Since in #2041907: Use some arcane magic to automatically discover typed data definitions for route parameters our plan is to make explicit type declaration not needed in 90+% of cases, I'm concerned about making them needed as an interim step, since we're technically past API freeze already, and routing.yml files are an API that affects nearly every contrib module. I think within this issue we should retain the name/type correspondence automagic, despite the flaws with it pointed out in #1906810-20: Require type hints for automatic entity upcasting, and remove it in #2041907: Use some arcane magic to automatically discover typed data definitions for route parameters. That way, we provide minimal disturbance to contrib modules who've begun early porting.
Why remove this test?
Here's a patch that addresses the above. I haven't yet reviewed the rest in depth, but wanted to get this up here in the meantime.
Also, removing the "needs tests" tag, since I think restoring the original tests, along with the new ParamConverterManagerTest, is sufficient. Please correct me though if you think there's something else that needs a test.
Comment #109
effulgentsia CreditAttribution: effulgentsia commentedOk, finished reviewing the rest. Only found nits.
Let's cast this to a boolean before returning it.
What about adding $value as a first parameter? That way, most converters wouldn't even need to deal with $defaults.
Since we're returning, we don't need the
?:
.Can we change $parameter to either $name or $parameter_name?
This seems wrong.
Comment #110
fubhy CreditAttribution: fubhy commentedOkay, makes sense to go with that temporary route alter solution.
Agreed with the review too. Fixed the stuff mentioned in #109.
Comment #111
effulgentsia CreditAttribution: effulgentsia commentedThanks. This looks great to me now, but due to #108, it would probably be best for someone else to RTBC.
Comment #112
damiankloip CreditAttribution: damiankloip commentedThis patch looks pretty cool. Here are a few things, shoot them down as necessary :)
Seems like we should settings on our naming conventions if we can. We currently mix underscores and '.' in service names. Out of scope of this issue, but probably worth talking about.
Does this mean it will call the method twice? or does PHP not do that? Would be nice if getOption() could specify a default, like ParameterBags can.
Use !empty() or something here? I'm not a fan of just using it bare, but this is not important :)
Is there a reason to do it this way? Seems more confusing using strlen() like this.
Maybe just 'Saves a list of applicable converters to each route.' would be better.
Here we should try to do something similar to #2012382: Improve efficiency of access checker matching on routes if possible, so we don't iterate over all the things. Or is this the other issue you mentioned? Sorry if it is.
Comment #113
fubhy CreditAttribution: fubhy commentedYeah, agreed. We might have 'static' converters. However, this is not as important as for access checks as we aim to cover 90% of the use-cases with the typed data converter.
I think it's fine as it is, especially because we are going to remove the EntityConverter entirely once the TypedDataConverter makes it in.
No, I am pretty sure that while there are other problems with ternaries it does not invoke the method twice. That would be beyond stupid. I am actually 100% it stores the return value in memory somewhere.
Yeah, but that's a general issue. We don't have a proper standard for service names. Definitely follow-up.
Comment #114
fubhy CreditAttribution: fubhy commentedWhoops, forgot to upload my patch.
Comment #115
damiankloip CreditAttribution: damiankloip commentedYeah, I pretty much meant that should be a follow up if it isn't already. That could unfortunately be an api change though :/
Comment #116
dawehnerFollow ups should be filled. Please just do it and link it in the summary.
As you said, 90% use-case is the typed data converter, so there will be simply not many data converters and based upon that, there is no real performance issue. No critique but it made the access manager not really more readable. Let's stay simple.
Comment #117
sdboyer CreditAttribution: sdboyer commentedRTBC +1 from me.
Comment #118
dawehnerAdding tag
Comment #119
pwolanin CreditAttribution: pwolanin commentedPossibly out of scope for this issue, but I'd like to see a follow-up where we do a better job of separating the "real" Request attributes that are being upcast here, and the extra ones like 'account' that we are sticking in there. We should really subclass Request to have a different property to hold those, or at least decide on a clear naming convention that will avoid collisions between the 2. I don't really think the symfony leading "_" pattern is very robust, since _ is valid in identifiers and path pattern slugs I think?
Comment #120
fubhy CreditAttribution: fubhy commentedRe-roll.
Comment #121
damiankloip CreditAttribution: damiankloip commentedReroll looks good.
Comment #122
Dries CreditAttribution: Dries commentedAwesome! Glad to see everyone agree. Committed to 8.x.
We still need to create the follow-up issues and create a change notifications so setting to 'active'.
Comment #122.0
vijaycs85Updated issue summary.
Comment #123
vijaycs85Updated follow up for service name standard #1943846-112: Improve ParamConverterManager and developer experience for ParamConverters in issue summary. We got few more follow up request, but not sure everyone agreed on them. Here is the list:
1. In #85
Follow up: This seems to be a really common pattern so somehow this could be moved to a common baseclass on the longrun.
2. In #85
Follow up: Throw exceptions in the entity manager instead.
Are they good/worth creating follow up? if not, we can remove 'Needs followup' tag.
Comment #124
xjmYep, worth filing issues. Also see #119, as well as #112 and following for the first snippet in that review.
In general, it would be good if patch authors got in the habit of filing followups right away based on feedback from their reviewers when they want to de-scope something from the issue. The issues don't have to be beautiful; If you're in a hurry, just reference the comment from the parent, quote enough of the issue that it makes sense, and tag it "Needs issue summary update". (If you're not in a hurry, take the time to explain the issue, so that we don't end up with issues that no one understands sitting around.)
Basically, what @dawehner said. :)
Comment #125
catch#2083415: [META] Write up all outstanding change notices before release
Comment #126
disasm CreditAttribution: disasm commentedChange notice started here: https://drupal.org/node/2084329
Disclaimer: I know absolutely nothing about this patch except for what I learned reverse engineering it fixing a WSCCI conversion. Change notice is as bare bones as it gets, but hopefully it helps someone else not spend a couple of hours trying to figure out how this works. That being said, it's been over a month since this patch got in. Can fubhy or someone else involved in this patch please get this change notice written up?
Comment #127
fubhy CreditAttribution: fubhy commentedYes, I will.. I am deeply sorry. I've been out of core Dev for a few weeks now as I am preparing myself for a major exam. Sorry for the delay...
Comment #127.0
fubhy CreditAttribution: fubhy commentedUpdated issue summary with follow up #2055857
Comment #128
xjmTagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release.
The patch for this issue was committed on July 24, 2013, meaning that the change record for this issue has been missing for more than six months.
Comment #129
dawehnerI don't really think that this issue is worth to change-notice, given that there was no real change for the outside world.
Comment #130
BerdirWe at least need documentation how it works and how you can add your own upcaster, and having a change notice for that is a first step to get someone to write documentation based on it :)
Comment #131
xjmTiny tag field--
Comment #132
dawehnerhttps://drupal.org/node/2217281
Comment #133
andypostThe change record https://drupal.org/node/2084329 filed at September 8, 2013
So the question is about "Needs followup" tag
Comment #134
andypost#127.0 already provides link to follow-up #2055857: [policy, then patch] Standardize service names
Comment #135
Wim Leershttps://drupal.org/node/2217281 is a 404 for me.
https://drupal.org/node/2084329 doesn't really qualify as a decent change notice to me. As #126 says, it's just an initial (rough) version.
Comment #136
xjmhttps://drupal.org/node/2084329 should be improved with whatever is needed. I deleted the draft at https://drupal.org/node/2217281 at @andypost's request; it was essentially just a duplicate of the other.