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 ???

CommentFileSizeAuthor
#78 drupal-1801356-78.patch15.95 KBamateescu
#78 interdiff.txt1.4 KBamateescu
#75 drupal-1801356-75.patch15.51 KBdawehner
#75 interdiff.txt5.03 KBdawehner
#73 drupal-1801356-73.patch10.75 KBamateescu
#73 interdiff.txt2.32 KBamateescu
#71 drupal-1801356-71.patch9.92 KBamateescu
#71 interdiff.txt4.73 KBamateescu
#60 drupal-1801356-60.patch10.34 KBdawehner
#53 drupal-1801356-53.patch15.5 KBdawehner
#49 drupal-1801356-49.patch15.78 KBdawehner
#42 drupal-1801356-42.patch11.93 KBdawehner
#42 interdiff.txt781 bytesdawehner
#40 drupal-1801356-40.patch11.97 KBdawehner
#40 interdiff.txt1.94 KBdawehner
#38 drupal-1801356-37.patch11.76 KBdawehner
#32 interdiff.txt566 bytesdawehner
#32 drupal-1801356-32.patch10.83 KBdawehner
#30 drupal-1801356-30.patch10.78 KBdawehner
#24 drupal-taxonomy_autocomplete-1801356-24-combined.patch23.13 KBdisasm
#24 drupal-taxonomy_autocomplete-1801356-24-review-do-not-test.patch10.13 KBdisasm
#24 interdiff.txt1.72 KBdisasm
#22 drupal-taxonomy_autocomplete-1801356-22-combined.patch23.14 KBdisasm
#22 drupal-taxonomy_autocomplete-1801356-22-review-do-not-test.patch10.14 KBdisasm
#22 interdiff.txt1.2 KBdisasm
#21 taxonomy-autocomplete-1801356-21.patch10.79 KBeffulgentsia
#21 interdiff.txt882 byteseffulgentsia
#18 taxonomy-autocomplete-1801356-18.patch10.61 KBeffulgentsia
#18 interdiff.txt1.64 KBeffulgentsia
#14 drupal-taxonomy_autocomplete-1801356-14.patch44.52 KBdisasm
#14 drupal-taxonomy_autocomplete-1801356-14-do-not-test.patch10.7 KBdisasm
#14 interdiff.txt471 bytesdisasm
#10 drupal-taxonomy_autocomplete-1801356-10-do-not-test.patch10.63 KBdisasm
#10 drupal-taxonomy_autocomplete-1801356-10-combined.patch19.5 KBdisasm
#10 interdiff.txt887 bytesdisasm
#8 drupal-taxonomy_autocomplete-1801356-8-do-not-test.patch10.88 KBdisasm
#8 drupal-taxonomy_autocomplete-1801356-8-combined.patch19.76 KBdisasm
#8 interdiff.txt9.99 KBdisasm
#5 drupal-taxonomy_autocomplete-1801356-2.patch9.46 KBdisasm
#5 interdiff.txt3.52 KBdisasm
#1 drupal-taxonomy_autocomplete-1801356-1.patch9.49 KBdisasm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

disasm’s picture

Status: Active » Needs review
FileSize
9.49 KB

attached is a patch from the routing-taxonomy-disasm branch of the WSCCI sandbox.

Crell’s picture

Issue tags: +WSCCI

Tagging, haven't looked at the code yet.

tim.plunkett’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TaxonomyRouteController.phpundefined
@@ -0,0 +1,111 @@
+use Drupal\taxonomy\Term;
+use Drupal\taxonomy\Vocabulary;
+use Symfony\Component\HttpFoundation\Response;

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -10,6 +10,10 @@ use Drupal\taxonomy\Term;
+use Symfony\Component\HttpFoundation\JsonResponse;

These aren't used at all

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TaxonomyRouteController.phpundefined
@@ -0,0 +1,111 @@
+    array_shift($args);

While refactoring, might as well make this unset($args[0]);, we've switch to only using array_shift() when you want the result.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TaxonomyRouteController.phpundefined
@@ -0,0 +1,111 @@
+      print t('Taxonomy field @field_name not found.', array('@field_name' => $field_name));
+      exit;

This was discussed in IRC, I think it was said it should be Response? Which invalidates part of my comment about the use statements :)

Status: Needs review » Needs work

The last submitted patch, drupal-taxonomy_autocomplete-1801356-1.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
3.52 KB
9.46 KB

Attached is a patch with Tim's requested changes and an interdiff to #1.

Status: Needs review » Needs work

The last submitted patch, drupal-taxonomy_autocomplete-1801356-2.patch, failed testing.

Crell’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TaxonomyRouteController.php
@@ -0,0 +1,110 @@
+    // If the request has a '/' in the search text, then the menu system
+    // will have split it into multiple arguments, recover the intended
+    // $tags_typed.
+    $args = func_get_args();
+    // Shift off the $field_name argument.
+    unset($args[0]);
+    $tags_typed = implode('/', $args);

Ah 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.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TaxonomyRouteController.php
@@ -0,0 +1,110 @@
+      $query = db_select('taxonomy_term_data', 't');
+      $query->addTag('translatable');
+      $query->addTag('term_access');
+

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.

disasm’s picture

I 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.

disasm’s picture

Ignore 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.

disasm’s picture

Removed disasm method, as well as two use statements that aren't used anymore in TaxonomyRouteController.

Crell’s picture

Component: wscci » routing system

Refiling.

Status: Needs review » Needs work

The last submitted patch, drupal-taxonomy_autocomplete-1801356-10-combined.patch, failed testing.

Crell’s picture

disasm’s picture

rebased 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.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-taxonomy_autocomplete-1801356-14.patch, failed testing.

effulgentsia’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Taxonomy.php
@@ -0,0 +1,126 @@
+/**
+ * Controller routines for taxonomy routes.
+ */
+class Taxonomy implements ContainerAwareInterface {

If 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.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Taxonomy.php
@@ -0,0 +1,126 @@
+  /**
+   * The injection container for this object.
+   *
+   * @var \Symfony\Component\DependencyInjection\ContainerInterface
+   */
+  protected $container;
+
+  /**
+   * Injects the service container used by this object.
+   *
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
+   *   The service container this object should use.
+   */
+  public function setContainer(ContainerInterface $container = NULL) {
+    $this->container = $container;
+  }

This belongs on the controller class, not the worker class.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Taxonomy.php
@@ -0,0 +1,126 @@
+  public function autocomplete($field_name, $tags_typed = '') {

If the class contains Autocomplete in its name as I suggest, then the method name could be something like getMatches().

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Taxonomy.php
@@ -0,0 +1,126 @@
+      return new Response(t('Taxonomy field @field_name not found.', array('@field_name' => $field_name)));

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.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Taxonomy.php
@@ -0,0 +1,126 @@
+      $query = db_select('taxonomy_term_data', 't');

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.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Taxonomy.php
@@ -0,0 +1,126 @@
+    return new JsonResponse($term_matches);

We should return $term_matches and let the controller be responsible for making it a JsonResponse.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.64 KB
10.61 KB

I 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.

Crell’s picture

Title: Taxonomy autocomplete using hook_route_info » Taxonomy autocomplete using routes

Retitling...

Status: Needs review » Needs work

The last submitted patch, taxonomy-autocomplete-1801356-18.patch, failed testing.

effulgentsia’s picture

Issue tags: +Needs tests
FileSize
882 bytes
10.79 KB

In 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.

disasm’s picture

Status: Needs review » Needs work

The last submitted patch, drupal-taxonomy_autocomplete-1801356-22-combined.patch, failed testing.

disasm’s picture

somehow I botched that upload. Here's the correct files.

disasm’s picture

Status: Needs review » Needs work

This is green, but manually testing doing the following the field isn't rendered as autocomplete:

  1. Install Drupal 8.x
  2. Apply combined patch in #24
  3. Add foo, bar, and foobar terms to Tags
  4. Edit Content Type Basic Page
  5. Add field field_taxonomy of term reference with autocomplete widget
  6. Add new Page

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.

Crell’s picture

Possibly 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.)

Crell’s picture

Oh, no, I'm wrong, it's this issue: #1845402: Update menu link access checks for new router

Bah.

chx’s picture

+    $this->container = $container;
+    $tags_typed = drupal_container()

well, 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.

Crell’s picture

chx 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.78 KB

Rerolled 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.

Status: Needs review » Needs work

The last submitted patch, drupal-1801356-30.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.83 KB
566 bytes

Let's fix that.

Status: Needs review » Needs work
Issue tags: -Needs tests, -WSCCI

The last submitted patch, drupal-1801356-32.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +WSCCI

#32: drupal-1801356-32.patch queued for re-testing.

damiankloip’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TaxonomyAutocomplete.phpundefined
@@ -0,0 +1,130 @@
+    $tags_typed = $this->request->query->get('q');

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TaxonomyRouteController.phpundefined
@@ -0,0 +1,41 @@
+  public function autocomplete($field_name, $tags_typed = '') {
...
+      $matches = $taxonomy->getMatches($field_name, $tags_typed);

I guess this $tags_typed param doesn't need to be here as it's getting it from the query in getMatches()?

Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TaxonomyAutocomplete.php
@@ -0,0 +1,130 @@
+   * Router callback to output JSON for taxonomy autcomplete suggestions
+   *

Calling 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.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TaxonomyAutocomplete.php
@@ -0,0 +1,130 @@
+  public function getMatches($field_name) {
+
+    $tags_typed = $this->request->query->get('q');

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.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TaxonomyRouteController.php
@@ -0,0 +1,41 @@
+/**
+ * Controller routines for taxonomy routes.
+ */
+class TaxonomyRouteController extends ContainerAware {

The class name shouldn't have "Route" in it. Just "TaxonomyController" or "TaxonomyAutocompleteController" or something like that.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TaxonomyRouteController.php
@@ -0,0 +1,41 @@
+  public function autocomplete($field_name, $tags_typed = '') {

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.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TaxonomyRouteController.php
@@ -0,0 +1,41 @@
+    $taxonomy = new TaxonomyAutocomplete($this->container->get('request'), $this->container->get('database'));

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.

dawehner’s picture

Status: Needs work » Needs review

So 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?

dawehner’s picture

FileSize
11.76 KB

And with a patch...

Crell’s picture

Status: Needs review » Needs work

Almost! 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.94 KB
11.97 KB

That's indeed magic.

Status: Needs review » Needs work

The last submitted patch, drupal-1801356-40.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
781 bytes
11.93 KB

That one now works.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

If bot likes, I likes.

ParisLiakos’s picture

dawehner’s picture

There 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.

Crell’s picture

Status: Reviewed & tested by the community » Postponed

Slight 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.

amateescu’s picture

Title: Taxonomy autocomplete using routes » Entity reference autocomplete using routes
Status: Postponed » Needs work

@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.

dawehner’s picture

Assigned: disasm » dawehner

Working on it.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.78 KB

This 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.

catch’s picture

+
+    // Register the page controller and autocompletion helper.
+    $container->register('entity_reference.controller', 'Drupal\entity_reference\EntityReferenceController')
+      ->addArgument(new Reference('entity_reference.autocompletion'));
+
+    $container->register('entity_reference.autocompletion', 'Drupal\entity_reference\EntityReferenceAutocompletion')
+      ->addArgument(new Reference('plugin.manager.entity'));
   }

Why are these registered as services? They seem a bit too specific to be services to me.

dawehner’s picture

Blocked on #1914180: Unbreak and cleanup the autocomplete widgets right now

Why are these registered as services? They seem a bit too specific to be services to me.

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.

amateescu’s picture

Component: routing system » entity_reference.module
Status: Needs review » Needs work
+++ b/core/modules/entity_reference/entity_reference.routing.yml
@@ -0,0 +1,15 @@
+entity_reference.autocomplete:
+  pattern: '/entity_reference/autocomplete/{type}/{field_name}/{entity_type}/{bundle_name}'
+  defaults:
+    _controller: 'entity_reference.controller:handleAutocomplete'
+    entity_id: ''
+  requirements:
+    _access: 'TRUE'
+
+entity_reference.autocomplete.single:
+  pattern: '/entity_reference/autocomplete/{type}/{field_name}/{entity_type}/{bundle_name}/{entity_id}'
+  defaults:
+    _controller: 'entity_reference.controller:handleAutocomplete'
+    entity_id: ''
+  requirements:

I 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' and type: '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?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/EntityReferenceAutocompletion.php
@@ -0,0 +1,117 @@
+class EntityReferenceAutocompletion {

How about EntityReferenceAutocomplete?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/EntityReferenceController.php
@@ -0,0 +1,82 @@
class EntityReferenceController {

And EntityReferenceAutocompleteController or EntityReferenceAutocompleteRouteController.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.5 KB
I 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' and type: 'tags' too.

The change in the current patch allows to just use a single route, which is pretty cool.

Also, entity_id is missing from the first route, and it needs to default to 'NULL'. Yes, a string :)

Yeah this confused me quite some time.

Why do we move the access check into the controller (in handleAutocomplete()) instead of handling it separately, like before?

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).

How about EntityReferenceAutocomplete?

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?

Status: Needs review » Needs work

The last submitted patch, drupal-1801356-53.patch, failed testing.

Crell’s picture

catch: 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.

sun’s picture

In 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?

Crell’s picture

Controller-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.

dawehner’s picture

Another, 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.

catch’s picture

I'd say we can start to worry about DIC scaling when we get into the thousands of services

That'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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.34 KB

Just 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.

Status: Needs review » Needs work

The last submitted patch, drupal-1801356-60.patch, failed testing.

Dries’s picture

I 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.

effulgentsia’s picture

My 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.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/EntityReferenceController.php
@@ -0,0 +1,89 @@
+    return new JsonResponse($matches);

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.

dawehner’s picture

@effulgentsia
Do you know whether there is an issue for _format already, I couldn't find one.

effulgentsia’s picture

Not a d.o. issue, but the Symfony PR is https://github.com/symfony/symfony/pull/5711.

effulgentsia’s picture

alippai’s picture

I think you shouldn't return with array of HTML strings (at least not without theme()) in getMatches().

dawehner’s picture

Well 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 :)

dawehner’s picture

I'm wondering whether we should just use the containerAware for now but setup the dependencies in the constructor, and switch over (if needed) later?

Crell’s picture

For 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.

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.73 KB
9.92 KB

I propose to go with this for now.

Edit: added emphasis :)

amateescu’s picture

Status: Needs review » Postponed

Or 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.

amateescu’s picture

FileSize
2.32 KB
10.75 KB

Here'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.

amateescu’s picture

Status: Postponed » Needs review

Should be ready to go now.

dawehner’s picture

Assigned: dawehner » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
5.03 KB
15.51 KB

Great, but let's also remove the old code :)

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/EntityReferenceBundle.phpundefined
@@ -19,9 +20,11 @@ class EntityReferenceBundle extends Bundle {
+    // with the dependency injection container.

That comment sounds odd, but sure this sounded odd before.

amateescu’s picture

Good point! RTBC++

webchick’s picture

Status: Reviewed & tested by the community » Needs work

As 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.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.4 KB
15.95 KB

Actually, we don't need those hook_menu items at all!

dawehner’s picture

Thanks for the rerole!

podarok’s picture

#78 looks good
+1RTBC

amateescu’s picture

Core committers usually process the queue in fifo order, so thanks for taking this issue out of their short term attention!

ParisLiakos’s picture

rofl

xjm’s picture

#78: drupal-1801356-78.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

While 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.

amateescu’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Modifying issue summary to update remaining tasks.