Problem/Motivation
Implement a simple JSON callback as hook_route_info to show a real world example of how to migrate a hook_menu entry to a route controller with methods.
Proposed resolution
Remove taxonomy/autocomplete from hook_menu in taxonomy module. Replace with hook_route_info and a TaxonomyRouteController class with a method autocomplete.
Remaining tasks
Implement access control once #1793520: Add access control mechanism for new router system is resolved.
Replace print;exit; for errors in autocomplete function with some sort of response object.
Replace db_select in Taxonomy class with dependency injected container.
Fix autocomplete_path to work with GET string instead of a path in the URL so '/' works in tags typed.
User interface changes
None
API changes
Behavior of autocomplete_path ???
Comment | File | Size | Author |
---|---|---|---|
#78 | drupal-1801356-78.patch | 15.95 KB | amateescu |
#78 | interdiff.txt | 1.4 KB | amateescu |
#75 | drupal-1801356-75.patch | 15.51 KB | dawehner |
#75 | interdiff.txt | 5.03 KB | dawehner |
#73 | drupal-1801356-73.patch | 10.75 KB | amateescu |
Comments
Comment #1
disasm CreditAttribution: disasm commentedattached is a patch from the routing-taxonomy-disasm branch of the WSCCI sandbox.
Comment #2
Crell CreditAttribution: Crell commentedTagging, haven't looked at the code yet.
Comment #3
tim.plunkettThese aren't used at all
While refactoring, might as well make this
unset($args[0]);
, we've switch to only using array_shift() when you want the result.This was discussed in IRC, I think it was said it should be Response? Which invalidates part of my comment about the use statements :)
Comment #5
disasm CreditAttribution: disasm commentedAttached is a patch with Tim's requested changes and an interdiff to #1.
Comment #7
Crell CreditAttribution: Crell commentedAh crap, it's one of those paths...
I don't believe the new router handles passing arbitrary tail values to the controller. It only passes named parameters. That's by design for security, I think. That means we need to either find some other way of hacking around that, or pass the autocomplete string in a GET query rather than in the URL. I like option 2, better. That would be why this is failing.
Make this class ContainerAware, and then you can call $this->container->get('database')->select() instead. That's cleaner and not subject to changing out from under you.
I'd also argue that the actual autocomplete logic should not be in the controller method itself. Controllers should be kept as really small glue code. Grab the data it needs, pass it to the object that actually does the work, return response. That may be for a follow up, though.
29 days to next Drupal core point release.
Comment #8
disasm CreditAttribution: disasm commentedI switched the behavior here to use the new YAML syntax instead of the router hook.
The do-not-test is the patch to review. I tried to get Dependency Injection working with the taxonomy class I created, but kept getting cannot call method get on non-object.
The combined contains effulgentsia's patch from #1801570: DX: Replace hook_route_info() with YAML files and RouteBuildEvent
This still has the same issues with the test failing if a '/' is in the string to match terms to. To fix that, as Crell mentioned above, the autocomplete FAPI widget needs to be refactored to handle a GET string instead of passing it in the path.
Comment #9
disasm CreditAttribution: disasm commentedIgnore the disasm method. Just saw I forgot to remove it. Was using it for testing the routes when my YAML file was broken. I'll pull it out in my next commit.
Comment #10
disasm CreditAttribution: disasm commentedRemoved disasm method, as well as two use statements that aren't used anymore in TaxonomyRouteController.
Comment #11
Crell CreditAttribution: Crell commentedRefiling.
Comment #13
Crell CreditAttribution: Crell commentedOpened related issue: #1827544: [Discussion] Drop automated passing of extra argument: Y/N
Comment #14
disasm CreditAttribution: disasm commentedrebased this patch off of #1793520: Add access control mechanism for new router system which is rebased off of #1801570: DX: Replace hook_route_info() with YAML files and RouteBuildEvent. Added a permission of 'access content' as a restriction. Test will still fail until #1831324: Form API #autocomplete_path JS and callbacks to use GET q= parameter instead of menu tail is resolved. Reviews should be based off of the do-not-test patch.
Comment #15
disasm CreditAttribution: disasm commentedComment #17
effulgentsia CreditAttribution: effulgentsia commentedIf we're gonna separate controllers from "worker" classes as Crell suggests in #7, then this is the worker class, so should not be referred to as a controller. Also, I think the class name should be more specific, like "TaxonomyAutocomplete". I'd even prefer just "Autocomplete" since we're already in the Drupal\taxonomy namespace, but our current coding standards discourage that as being too ambiguous.
This belongs on the controller class, not the worker class.
If the class contains Autocomplete in its name as I suggest, then the method name could be something like getMatches().
We don't want to return a response here, but rather throw an exception. The controller should catch the exception and turn it into a Response.
Rather than calling db_select(), this class needs a constructor that receives a $connection parameter. When the controller instantiates this class, it should pass $this->container->get('database') as that parameter.
We should return $term_matches and let the controller be responsible for making it a JsonResponse.
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedI suggested in #1793520-117: Add access control mechanism for new router system that we should get this issue in before that one, so here's #14's "do not test" patch adjusted for that. The interdiff is relative to that one. "needs review" for bot, but #17 still needs doing, and it might still fail due to #1831324: Form API #autocomplete_path JS and callbacks to use GET q= parameter instead of menu tail.
Comment #19
Crell CreditAttribution: Crell commentedRetitling...
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedIn local testing of node/add/article, #18 wasn't turning the Tags widget into an autocomplete field at all. Here's a fix for that, but without a corresponding test addition to testTermAutocompletion(), so tagging.
Comment #22
disasm CreditAttribution: disasm commentedRebase off of #1831324: Form API #autocomplete_path JS and callbacks to use GET q= parameter instead of menu tail. Now using GET query in autocomplete.
Comment #24
disasm CreditAttribution: disasm commentedsomehow I botched that upload. Here's the correct files.
Comment #25
disasm CreditAttribution: disasm commentedThis is green, but manually testing doing the following the field isn't rendered as autocomplete:
Result: taxonomy reference input has no autocomplete class.
The issue is in form.inc form_process_autocomplete() it calls drupal_valid_path. drupal_valid_path doesn't know about new router paths. I verified this by deleting the drupal_valid_path() call from the function, but this probably needs refactored in another issue.
Comment #26
Crell CreditAttribution: Crell commentedPossibly related: #1843220: Convert form AJAX to new AJAX API
(We started messing with the ajax system recently, so it's possible something is wonky in form/ajax handling right now.)
Comment #27
Crell CreditAttribution: Crell commentedOh, no, I'm wrong, it's this issue: #1845402: Update menu link access checks for new router
Bah.
Comment #28
chx CreditAttribution: chx commentedwell, this doesn't seem correct or logical. Also, why does it need a whole container, wouldnt just the request do?? But then later + $taxonomy = new Taxonomy(); without a container. If this is an API restriction then we need to fix the routing API to pass in request, calling drupal_container() in a class makes my skin crawl, I can imagine Crell shuddering at it.
Comment #29
Crell CreditAttribution: Crell commentedchx is correct. In fact since the controller class her is ContainerAware, $this->container is already available and we don't need a constructor for that.
Comment #30
dawehnerRerolled to let the router controller be container aware and the helper class to be fully injected.
The last patch had quite a lot of new changes in there.
Comment #32
dawehnerLet's fix that.
Comment #34
disasm CreditAttribution: disasm commented#32: drupal-1801356-32.patch queued for re-testing.
Comment #35
damiankloip CreditAttribution: damiankloip commentedI guess this $tags_typed param doesn't need to be here as it's getting it from the query in getMatches()?
Comment #36
Crell CreditAttribution: Crell commentedCalling this a "router callback" is a misnomer, since it's not connected to the router at all (or shouldn't be). Really, this object is a service for doing fancy lookups on the taxonomy system; basically it has extra utility methods that, in Symfony, would just get put on the Entity Manager (what we currently call Storage Controller). The class/method should be named accordingly.
The query string itself should be passed into the method, not via a stored request. We're not using the request here, just the query string, so don't couple this object to the request. That means we can also remove the request from the constructor.
The class name shouldn't have "Route" in it. Just "TaxonomyController" or "TaxonomyAutocompleteController" or something like that.
The controller shouldn't get the request from the container. It should have Request $request as a method parameter, in which case it will get passed in automagically.
Also, $tags_typed doesn't need a default value here. Default values should be handled in the route definition. And, actually, it looks like that value is unused anyway since we're using $request->get('q'). Just use that and eliminate the extra parameter.
TaxonomyAutocomplete should be in the DIC so that it can be cleanly loaded, and swapped out. If taxonomy terms are not stored in the database, this code will fail. Also, it doesn't need a request, just the query string to match.
Comment #37
dawehnerSo what about something like that, two service which are used, no containerAware class and everything get's injected.
Should we add explicit unit testing for TaxonomyAutocomplete?
Comment #38
dawehnerAnd with a patch...
Comment #39
Crell CreditAttribution: Crell commentedAlmost! The Request $request parameter should be on the controller method (autocomplete()), not as a constructor dependency. The ControllerResolver will pass the request into the controller method directly if it is listed there.
Otherwise I think this is good.
Comment #40
dawehnerThat's indeed magic.
Comment #42
dawehnerThat one now works.
Comment #43
Crell CreditAttribution: Crell commentedIf bot likes, I likes.
Comment #44
ParisLiakos CreditAttribution: ParisLiakos commentedi guess its too late but #1847596: Remove Taxonomy term reference field in favor of Entity reference
Comment #45
dawehnerThere is another issue that tries to replace the field_name parameter with the vocabulary id, which is a requirement for views to get rid of it's custom autocomplete function, so the other issue is not needed anymore, from what I understand.
Comment #46
Crell CreditAttribution: Crell commentedSlight change of plans. I talked to catch, and we realized this issue is going to conflict with #1847596: Remove Taxonomy term reference field in favor of Entity reference. That's pretty guaranteed to happen one way or another, so we'll let that issue handle doing an ER-autocomplete conversion rather than a taxonomy-specific conversion.
Marking postponed, just in case, but it will probably become a won't fix or else mutate once the other issue is in.
Comment #47
amateescu CreditAttribution: amateescu commented@Crell liked my idea from #1847596-57: Remove Taxonomy term reference field in favor of Entity reference, so we have nothing to wait for here! Let's just change the patch to convert ER's autocomplete instead.
Comment #48
dawehnerWorking on it.
Comment #49
dawehnerThis will make #1169964: Taxonomy autocomplete: decouple from Field API and factor out a helper so that code is reusable way harder, so we still need some special treatment for vocabulary-only autocompletion.
Here is a first version of it.
Comment #50
catchWhy are these registered as services? They seem a bit too specific to be services to me.
Comment #51
dawehnerBlocked on #1914180: Unbreak and cleanup the autocomplete widgets right now
So what I understand about the DIC is that is allows to inject dependencies into the object, and describe them. The UserBundle does the same now for it's autocompletion.
Comment #52
amateescu CreditAttribution: amateescu commentedI don't know very much about the new way of declaring routes, but looking at the $entity_id part it looks like we can hardcode
type: 'single'
andtype: 'tags'
too.Also, entity_id is missing from the first route, and it needs to default to 'NULL'. Yes, a string :)
Why do we move the access check into the controller (in
handleAutocomplete()
) instead of handling it separately, like before?How about
EntityReferenceAutocomplete
?And
EntityReferenceAutocompleteController
orEntityReferenceAutocompleteRouteController
.Comment #53
dawehnerThe change in the current patch allows to just use a single route, which is pretty cool.
Yeah this confused me quite some time.
I tried it for some time, but you don't really get the values from the urls (at least I didn't managed to do it, and klausi told me that then I should do something like that).
Fixed that, but not the other issue. As long we don't need separate controller for different kind of pages, I think we should keep one.
BTW: Do we really need test for this issue, doesn't ER provide some of them already?
Comment #55
Crell CreditAttribution: Crell commentedcatch: The DIC allows for services to be listed as "private", meaning they don't show up in lists of available services but you can still wire them up to be injected. We're not using that yet, but there's no doubt plenty of places we could. (I think it's even possible to have names auto-generated, but that may be specifically a Symfony fullstack thing.) A service can be a service even if it's not something intended to be offered to the entire system.
Comment #56
sunIn that case, the controller still does not need to be service though. Only the autocompletion thingie wants to be one, since it gets the entity manager injected. The controller itself does not need dependency injection?
In general, @catch brought up a great question there. The more stuff we convert to controllers, the more often we'll actually run into this topic. And, off-hand, I'd agree that it doesn't make sense to register every controller plus every handler class behind the controller as a separate DI service. Considering a full-blown Drupal production site, that will most likely get us into completely different performance territories for which the DI container wasn't designed for.
So, in short, the DI facility is nice and all, but I've the impression we're over-abstracting. If I don't like it and need to swap out entity_reference, then I can always swap out the entire module?
Comment #57
Crell CreditAttribution: Crell commentedController-as-service is fully supported. The Symfony world from what I've seen doesn't have a firm consensus on container aware controllers vs. service controllers. The general feeling seems to be that service controllers are more formal and clean and loosely coupled, but controllers are one of the few areas where making something container aware isn't a huge deal. (Fabpot is actually in the latter camp, yet he wrote the Drupal support for service controllers. Go figure. :-) ) The Symfony FrameworkBundle also includes a base controller class that is container aware and includes a number of convenience methods.
Personally I'd favor service controllers in most cases, but not all. What I've been advising people is to start with a container aware controller when writing it originally, for simplicity, and then when you know what dependencies you'll actually have convert it t a service with just those dependencies. Cases like splitting the controller from the autocomplete lookup code, for instance, are kind of muddy. In Symfony fullstack, such lookup methods would go on the Entity Manager class for the repository (roughly equivalent to our Entity Storage Controllers), and that's the ONLY place you should ever have queries that touch the data store. Of course, that's far more feasible when you assume all changes involve coding, which Symfony fullstack can and does while Drupal can't and does not.
I think that's an area where we just don't have enough practical experience yet to have a clear recommended best practice. We'll probably figure that out somewhere in June once we've done more conversions and have a better grasp of what is worth separating out.
The DIC, once compiled, scales pretty well. From my conversations with Lukas Smith on the subject, I'd say we can start to worry about DIC scaling when we get into the thousands of services. Until then, I wouldn't worry.
Comment #58
dawehnerAnother, but probably architectural much worse way, would be to use the Autocomplete helper class directly, and the controller on the routing definition as well.
If people then would want to swap the behaviour, they would always have to change the routing definition. I don't think this is a good way to go.
Comment #59
catchThat's about how many router items there are on lots of Drupal 7 sites (and Drupal 6), so we have to worry about that right now. If we wrote our own router because we didn't think Symfony's would scale to hundreds or thousands of router items, then I see no reason why we should optimistically put them in the DIC 'until it becomes a problem'.
Also #1914106: Use EntityQuery for user autocomplete is a good example of what happens when you want change internal implementation from a DX point of view if things are wired up in the container.
Comment #60
dawehnerJust rerolling the patch against the latest changes in the autocompletion code.
I'm wondering whether we should maybe split up the autocompletion logic a bit different. It seems to be that you want to separate getMatches from constructing HTML.
Comment #62
Dries CreditAttribution: Dries commentedI can go with either approach, but I'd really like core to be consistent and use one approach, instead of mixing two approaches. Making the controller as a service makes its dependencies more explicit and therefore easier for someone reading the code, writing unit tests (assuming we'd want to), etc. I honestly need to think about it a bit more, but that is where I'm at at the moment. I'm certainly sensitive to the performance fears. As a result, I'm kinda on the fence.
Comment #63
effulgentsia CreditAttribution: effulgentsia commentedMy opinion at this time (and quite subject to being changed) is that controllers shouldn't be DIC services. But HEAD currently does both, so it shouldn't be this issue's scope to settle that debate. Instead, I opened #1915774: Decide whether core route controllers should generally/always be DIC services or not. I tried to be fair in the issue summary, but given my bias, others here may want to look that over and improve it.
Meanwhile, the only other autocomplete route we have in core that's using the new routing system is in user.routing.yml, and that one uses a controller as a service. Based on that, I think it's reasonable for this issue to proceed with that pattern until we settle on something in the other issue.
We're returning a JsonResponse, but not checking anywhere that the user agent can accept that. I think the eventual plan is for route requirements to include a
_format
option, so that a content negotiation library can exclude routes that return JSON from requests that don't want that, but maybe that's not ready yet. Just noting that here in case it helps us collectively remember to add that in when our content negotiation library is in place.Comment #64
dawehner@effulgentsia
Do you know whether there is an issue for _format already, I couldn't find one.
Comment #65
effulgentsia CreditAttribution: effulgentsia commentedNot a d.o. issue, but the Symfony PR is https://github.com/symfony/symfony/pull/5711.
Comment #66
effulgentsia CreditAttribution: effulgentsia commentedThe closest d.o. issue is probably #1505080: [META] Content Negotiation for Accept Headers.
Comment #67
alippai CreditAttribution: alippai commentedI think you shouldn't return with array of HTML strings (at least not without theme()) in getMatches().
Comment #68
dawehnerWell you are right, but this issue is actually just about getting the conversion done, not about fixing the code in general?
To be honest: I don't know the reason why we need html here, but well, there has to be a good reason :)
Comment #69
dawehnerI'm wondering whether we should just use the containerAware for now but setup the dependencies in the constructor, and switch over (if needed) later?
Comment #70
Crell CreditAttribution: Crell commentedFor now, just do whatever you feel like doing. We can standardize things when #1915774: Decide whether core route controllers should generally/always be DIC services or not works itself out, but let's not block any patches on that. Full speed ahead here with whichever approach you feel like using.
Comment #71
amateescu CreditAttribution: amateescu commentedI propose to go with this for now.
Edit: added emphasis :)
Comment #72
amateescu CreditAttribution: amateescu commentedOr not. In today's WSCCI meeting it was agreed that the solution from #1915774-5: Decide whether core route controllers should generally/always be DIC services or not is the best idea for now, so postponing on that.
Comment #73
amateescu CreditAttribution: amateescu commentedHere's a new patch based on the
ControllerInterface
approach from #1915774-17: Decide whether core route controllers should generally/always be DIC services or not. Leaving as postponed until that issue is fixed.Comment #74
amateescu CreditAttribution: amateescu commentedShould be ready to go now.
Comment #75
dawehnerGreat, but let's also remove the old code :)
That comment sounds odd, but sure this sounded odd before.
Comment #76
amateescu CreditAttribution: amateescu commentedGood point! RTBC++
Comment #77
webchickAs of ~Friday, we no longer do that NOT USED crap, and instead add a pointer to the route machine name. See #1845402: Update menu link access checks for new router.
Comment #78
amateescu CreditAttribution: amateescu commentedActually, we don't need those hook_menu items at all!
Comment #79
dawehnerThanks for the rerole!
Comment #80
podarok#78 looks good
+1RTBC
Comment #81
amateescu CreditAttribution: amateescu commentedCore committers usually process the queue in fifo order, so thanks for taking this issue out of their short term attention!
Comment #82
ParisLiakos CreditAttribution: ParisLiakos commentedrofl
Comment #84
xjm#78: drupal-1801356-78.patch queued for re-testing.
Comment #85
webchickWhile testing this I ran into #1956434: Can't edit nodes with entity reference fields. :( However, this patch doesn't seem to make that any worse.
Committed and pushed to 8.x.
Comment #86
amateescu CreditAttribution: amateescu commentedI opened a new issue for the problem found in #67: #1959116: Entity reference autocomplete shouldn't include html in its return value
Comment #87.0
(not verified) CreditAttribution: commentedModifying issue summary to update remaining tasks.