Entities can provide route parameters for their routes. Parameters you can query by calling $entity->toUrl($link_relation)->getRouteParameters(); Yet, Field UI chooses to ignore those and only hard-codes specific route parameters.

A great example of where it crashes the site is field_ui_entity_operation():

$operations['manage-fields'] = array(
        'title' => t('Manage fields'),
        'weight' => 15,
        'url' => Url::fromRoute("entity.{$bundle_of}.field_ui_fields", array(
          $entity->getEntityTypeId() => $entity->id(),
        )),
      );

This only works if the 'field_ui_base_route' path is something like my_entity/{my_entity}. If it's anything like my_module/{some_context}/my_entity/{my_entity}, it will crash because {some_context} is not being provided.

In order to fix this, we need to make Field UI define link relations and properly name its routes after said link relations. It currently has some routes that do not follow the (albeit undocumented) convention of entity.{entity_type_id}.link_relation_name.

The most recent patch approach does this in a backwards-compatible way.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kristiaanvandeneynde created an issue. See original summary.

kristiaanvandeneynde’s picture

Status: Active » Needs review
FileSize
4.29 KB

Attached is a patch making the method public, documenting it on the interface and implementing it in field_ui_entity_operation(). The last bit should make sure it gets covered by more tests.

Status: Needs review » Needs work

The last submitted patch, 2: drupal-entity_route_params-2651974-2.patch, failed testing.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
4.96 KB

Forgot to update a method signature...

dawehner’s picture

Making it public would be certainly kind of an API break, wouldn't it? Existing implementations would break.

Status: Needs review » Needs work

The last submitted patch, 4: drupal-entity_route_params-2651974-4.patch, failed testing.

kristiaanvandeneynde’s picture

@dawehner Well, yes and no... As it stands, the method is only used in Entity because it's protected. Core only has one implementation (FieldConfig), which makes it rather unlikely that there are more implementations. Unlikely, not impossible.

There are numerous use cases which are currently hard or impossible to implement because that method is not public. The best example being an entity which has to be visited in a context path as shown in the issue summary. So perhaps breaking the API here for the sake of allowing all of these use cases isn't such a bad thing?

Implementing another public method with the sole purpose of calling urlRouteParameters() seems a bit daft, but would not cause an API break if that's really an issue.

kristiaanvandeneynde’s picture

Attached is a patch which fixes the method for ViewUI. If it passes all tests, I'll work on a patch which actually wraps the method with a getRouteParameters() function. This will then not break API, and it turns out it isn't as daft as I said it was in #7 because it's already being done on Entity for uriRelationships() and linkTemplates().

dawehner’s picture

@dawehner Well, yes and no... As it stands, the method is only used in Entity because it's protected. Core only has one implementation (FieldConfig), which makes it rather unlikely that there are more implementations. Unlikely, not impossible.

Well, i could actually imagine that contrib overrides that and then we have a problem.

kristiaanvandeneynde’s picture

So a wrapper it is as suggested in #8?

dawehner’s picture

@kristiaanvandeneynde How would you do that without any additional method?

kristiaanvandeneynde’s picture

@dawehner: I couldn't. That's what I've been talking to myself about in #7 and #8 :)

Implementing another public method with the sole purpose of calling urlRouteParameters() seems a bit daft, but would not cause an API break if that's really an issue.

... I'll work on a patch which actually wraps the method with a getRouteParameters() function. This will then not break API ... it's already being done on Entity for uriRelationships() and linkTemplates().

kristiaanvandeneynde’s picture

Title: Entity::urlRouteParameters() should be public (and implemented by field_ui_entity_operation()) » Expose Entity::urlRouteParameters() and implement it in field_ui_entity_operation()
FileSize
4.54 KB

Here's what I had in mind. No interdiff because it is a different approach than what I've posted before.

kristiaanvandeneynde’s picture

Also, check out the patch in #2645136: Clearly document the expected route name pattern for entities.

Combined, the two would provide a getRouteName() and getRouteParameters() on all entities so that other modules may easily find out what routes entities provide for relationships and what parameters those routes require. I really think this would prove a valuable addition to the Entity API.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -321,6 +321,13 @@ protected function urlRouteParameters($rel) {
    +  public function getRouteParameters($rel) {
    +    return $this->urlRouteParameters($rel);
    

    Why not just make the other method public?

    EDIT: I see that's the difference between #8 and #13

  2. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -230,6 +230,20 @@ public function toLink($text = NULL, $rel = 'canonical', array $options = []);
    +  public function getRouteParameters($rel);
    

    We can't do this in D8 though. The ViewUI change shows exactly why: if you implement EntityInterface directly without extending the base class, you'll get a fatal error.

I'm not sure this can be done in a BC safe way, but this would be really helpful for config translation as well.

kristiaanvandeneynde’s picture

Thanks for your input Tim.

I agree there isn't a 100% fool-proof way of adding this without breaking BC. Although adding a signature to an interface should break a lot less modules than changing the visibility of an existing method on the interface/base class.

In core, it only needed an update for Views which doesn't extend the base class. In contrib, I can't imagine there are that many module developers masochistic enough to want to write their entire entity implementation from scratch.

kristiaanvandeneynde’s picture

Crell’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: +Needs architectural review

The minor API change here would push this to 8.1. Also tagging for the Alexes to weigh in on that front.

kristiaanvandeneynde’s picture

I can live with 8.1.x, thanks for the review :)

dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -230,6 +230,20 @@ public function toLink($text = NULL, $rel = 'canonical', array $options = []);
+   */
+  public function getRouteParameters($rel);

getUrlParameters is clearly not a classic getter, so maybe it should not be named like that.

kristiaanvandeneynde’s picture

I'm not sure I'm following, there is no getUrlParameters() :s

My patch introduces getRouteParameters(), which does exactly that: getting the route parameters. It calls urlRouteParameters() internally, but that is none of my doing. That method was already in core and actually returns the route parameters as well.

dawehner’s picture

My patch introduces getRouteParameters(), which does exactly that: getting the route parameters. It calls urlRouteParameters() internally, but that is none of my doing. That method was already in core and actually returns the route parameters as well.

Right, why do you not just make urlParameters() public and call it a day?

kristiaanvandeneynde’s picture

Because of what you said in #5? :D

You were afraid of breaking BC, so I added a wrapper function which means any implementation of urlParameters() will keep on working while most entities will not break with the addition of getRouteParameters(). The only ones that will break are the ones that do not extend the Entity base class.

We could do what core seems to have done for a lot of things which needed renaming: wrap the old function in a new one (done) and mark the old one as deprecated (not done).

dawehner’s picture

OH, mh, sorry for the noise.

kristiaanvandeneynde’s picture

No problem, it's Friday after all :)

So does this mean RTBC or does the architectural review need to come first?

tim.plunkett’s picture

kristiaanvandeneynde’s picture

Status: Needs review » Reviewed & tested by the community

8.1.x will be closed in little over a week (https://groups.drupal.org/node/508968), let's not make this have to wait another couple of months please.

Setting to RTBC as no-one seems to have any objections to the patch presented. Crell did mention it needs an architectural review in #18, though.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review

Your impatience does not allow you to RTBC your own patches.
This was *just* explained to you in #2645136-18: Clearly document the expected route name pattern for entities

kristiaanvandeneynde’s picture

I just realized I shouldn't be putting this to RTBC myself. Please see #2645136-22: Clearly document the expected route name pattern for entities as to why I did.

kristiaanvandeneynde’s picture

@tim.plunkett, I was just setting this issue back to needs review for the exact same reason.

Edit: I marked both issues as RTBC at the exact same time. So it's not like I did this after I was told not to.

tim.plunkett’s picture

Thanks for clarifying :)

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kristiaanvandeneynde’s picture

So is there anything holding us back still?

Tests went green, it doesn't break BC by changing a function signature to become public (which would break a lot of contrib entity types). Instead, it introduces a new public method which is added to the base Entity class so almost all contrib entities won't even notice the change. Only those that implement EntityInterface directly (which I can't imagine being a lot of modules).

Does that make the change small enough to warrant a change record and commit it to 8.2.x?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kristiaanvandeneynde’s picture

Just had an awesome idea that also relies on this patch: #2791571: Automatically supply contextual links for entities

Perhaps the patch from this issue should split off the field_ui part into a PP-1 issue as well or is it fine to stay as is?

kristiaanvandeneynde’s picture

I discussed this with dawehner on IRC and we thought it would be great to have the Url generated by $entity->toUrl() to be the single source of truth. So you could ask it for its route parameters for example.

Knowing that, I tried to fix the current issue by making Field UI add link relations/templates to the entities it's altering but that turns out to be impossible to do without changing routes.

See attached patch: It registers link relations such as drupal:field-ui-fields, but they can never match the routes Field UI defines because they get automatically translated to entity.ENTITY_TYPE_ID.RELATION:

  • Relation: drupal:field-ui-fields
    Becomes: entity.BUNDLE_ENTITY_TYPE_ID.field_ui_fields.
    Actual route: entity.BUNDLEABLE_ENTITY_TYPE_ID.field_ui_fields
  • Relation: drupal:field-ui-form-display
    Becomes: entity.BUNDLE_ENTITY_TYPE_ID.field_ui_form_display.
    Actual route: entity.entity_form_display.BUNDLEABLE_ENTITY_TYPE_ID.default
  • Relation: drupal:field-ui-display
    Becomes:entity.BUNDLE_ENTITY_TYPE_ID.field_ui_display.
    Actual route: entity.entity_view_display.BUNDLEABLE_ENTITY_TYPE_ID.default

See: \Drupal\field_ui\Routing\RouteSubscriber::alterRoutes()

So we either need to make the attached patch work somehow, even though changing existing route names may count as a BC break. Or we need to go ahead with the original idea of exposing the route parameters because there is no other way Field UI can work properly. Even if that means $entity->toUrl() will not be the single source of truth.

Status: Needs review » Needs work

The last submitted patch, 36: drupal-entity_route_params-2651974-36.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

grathbone’s picture

Unsure as to whether this should be another issue or not, but it's related to getRouteParameters().

Any chance we could throw in an moduleHandler->alter here, similar to how getOperations runs an alter on defaultOperations before returning them?

$parameters = $this->urlRouteParameters($rel);
$this->moduleHandler->alter('entity_route_parameters', $parameters, $entity, $rel);
return $parameters;
kristiaanvandeneynde’s picture

Status: Needs work » Needs review

Not really related. This issue is about exposing the route params that are provided by the entity class itself.

Summary:
As demonstrated by this issue and #2670712: ConfigEntityMapper::getBaseRouteParameters() should consult Entity::urlRouteParameters(), there might be a need to be able to read the route parameters an entity expects. This is currently possible using $entity->toUrl()->getRouteParameters(). This is arguably silly as we are basically using a bypass to access a method that should have been public to begin with. There might not be a need to run toUrl() so doing so only to access the route parameters is just wasting resources.

The upside is that it would not break BC whatsoever. It would just look like an ugly hack :)

Proposal:
Having re-read the issue and knowing what I know now, I would still recommend an approach to the likes of #13.

  1. Core already uses a method named getRouteParameters() elsewhere, so we'd be normalizing the name.
  2. It does not make much sense for entities to extend the EntityInterface directly. We even encourage people to extend base classes over entity interfaces (see #2661926: [policy no-patch] Document that interfaces without an @api tag can have API additions during a minor release).
  3. That said, it is possible that people did extend it directly, as demonstrated by ViewUI. Although I'd argue that class is an accident waiting to happen, given how many methods it wraps 1:1.

So I see two solutions:

  • Go ahead with the patch from #13 and write up a CR indicating the change. Given point 2 above, the impact should be minimal.
  • Consider point 3 above as a strong reason to approach this differently. Introduce a new interface ExposedRouteParametersInterface and check for that in field_ui_entity_operation() and other places. Immediately mark the interface as deprecated for 9.0.x and also deprecate Entity::urlRouteParameters() in favor of ExposedRouteParametersInterface::getRouteParameters() which should be moved to EntityInterface in 9.0.x

Using the latter solution might be a less ugly than $entity->toUrl()->getRouteParameters() in the long run, as it would be easier to convert it to a public method on EntityInterface in Drupal 9.

Setting back to Needs Review for the patch in #13. If people want to go ahead with the latter solution, please set it to Needs Work.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kristiaanvandeneynde’s picture

Title: Expose Entity::urlRouteParameters() and implement it in field_ui_entity_operation() » field_ui_entity_operation() cannot respect route parameters because of incorrectly named routes
Component: entity system » field_ui.module
Issue tags: -Needs architectural review
FileSize
7.74 KB

Okay so the real issue here is the one outlined in #36: Field UI does not name its routes correctly to match the (undocumented) convention regarding route naming found in EntityInterface::toUrl().

In order to fix this, I have added the correct route names as duplicates of the current ones and added a route filter that makes sure the old one will still be selected by Drupal. This will make sure there is 100% backwards compatibility, but at the same time allow us to use $entity->toUrl('drupal:field-ui-form-display') to make sure the generated URL has the correct route parameters.

This is the need-to-change only MVP patch. We might want to update all Field UI route names that violate the convention in either this patch or a follow-up issue.

Status: Needs review » Needs work

The last submitted patch, 42: drupal-2651974-42.patch, failed testing. View results

kristiaanvandeneynde’s picture

Oh wow, this is horrible.

So ModuleInstaller swaps out the route builder with the lazy route builder, which eventually triggers the error by calling rebuild() on an already rebuilding route builder. But how we get there is really weird.

With this patch (what patch influences is in bold):

  1. ModuleInstaller builds the entity types
  2. The entity types are built and handed off to alter hooks
  3. Field UI asks the lazy route builder for the entity type's Field UI base route
  4. The lazy route builder triggers a rebuild in the route builder
  5. All of the route subscribers are called, including Views' \Drupal\views\EventSubscriber\RouteSubscriber
  6. This route subscriber's constructor asks the entity manager for the view storage handler
  7. In order to be able to do so, the entity types need to be built. But they hadn't finished building yet.
  8. A new entity type (re)build is therefore triggered
  9. This gets us back to Field UI's entity type alter hook
  10. This leads to another route request, throwing the exception shown in the test results: RuntimeException: Recursive router rebuild detected.

If it hadn't been for the above exception being thrown, we would have wound up in an infinite loop.

So the question is: Who is doing it wrong?

  • Is my patch wrong for asking for a route in the entity type alter hook? It seems like a sensible thing to do.
  • Is views wrong for asking for an entity storage handler in a route subscriber? It doesn't seem wrong either to do that. This is the root cause of an infinite loop, though, if any code that runs during entity type building tries to ask for a route.
kristiaanvandeneynde’s picture

Seems to me like this is an inherent problem with the entity type manager and by extension all plugin managers. All it takes for an infinite loop to occur is for one entity type build (alter) hook to invoke a system which has a listener that requires an entity type. In this case, the routing system with Views' route subscriber.

Said route subscriber is harmless until it gets invoked during an entity type rebuild. But the same goes for any system. You could have totally harmless implementations until someone decides they need that system in hook_entity_type_alter(). Then it causes an infinite loop because the entity type manager will keep rebuilding over and over again.

kristiaanvandeneynde’s picture

Issue summary: View changes

Updated the IS to reflect the new direction this issue has taken.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
8.77 KB
2.77 KB

Just seeing if a simple recursion prevention would work. It's obviously not the right solution to the bigger problem, but at least it might help me polish this patch further without "unrelated" test failures.

Status: Needs review » Needs work

The last submitted patch, 47: drupal-2651974-47.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kristiaanvandeneynde’s picture

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
10.68 KB

This still needs tests, but abandons trying to read the path from the Field UI base route (because race conditions) in favor of a field_ui_base_path entity type key. Let's see how many things this breaks now.

kristiaanvandeneynde’s picture

  1. +++ b/core/modules/field_ui/field_ui.module
    @@ -10,7 +10,7 @@
    +use Drupal\Core\Routing\RouteProviderLazyBuilder;
    

    No need for this any more. Will clean up after tests have finished running.

  2. +++ b/core/modules/field_ui/src/Routing/DeprecatedRouteMatcher.php
    @@ -0,0 +1,55 @@
    + * @deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0. Remove
    

    Should say Drupal 8.7.0

Status: Needs review » Needs work

The last submitted patch, 52: drupal-2651974-52.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

kristiaanvandeneynde’s picture

Are the tests failing because of the newly introduced deprecation markers?

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
11.88 KB
2.84 KB

Let's try this.

Status: Needs review » Needs work

The last submitted patch, 56: drupal-2651974-56.patch, failed testing. View results

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
12.94 KB
3.09 KB

Whoops, forgot to keep using the old URL-generating logic for entity types without a Field UI base path.

kristiaanvandeneynde’s picture

Cool, all green! Keep in mind that the new behavior might still need a few tests.

tstoeckler’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -241,7 +241,22 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
    +   * @todo From Drupal 9.0.0 onwards, this should be used to detect whether an
    +   * entity type supports Field UI.
    ...
    +   * @todo From Drupal 9.0.0 onwards, this should only be used to detect whether
    +   * an entity type supports Field UI local tasks (tabs).
    

    Interesting. Is this distinction temporary, i.e. do we want to move the local tasks detection to use the path in the future, as well, or not?

  2. +++ b/core/modules/field_ui/field_ui.module
    @@ -105,6 +105,31 @@ function field_ui_entity_type_build(array &$entity_types) {
    +    // If the entity type uses another entity type as its bundle, we need to
    +    // set the Field UI link templates on the bundle entity type instead.
    

    Really? That is a bit unintuitive, in my opinion. So far we have always treated the field UI link templates as belonging to the entity type that is being configured, not the bundle entity type. Can you elaborate why this is necessary?

  3. +++ b/core/modules/field_ui/field_ui.module
    @@ -145,30 +170,42 @@ function field_ui_entity_operation(EntityInterface $entity) {
    +      // @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0.
    +      // Should always use $entity->toUrl() starting from Drupal 9.0.0 onwards.
    +      $url = $entity->hasLinkTemplate('drupal:field-ui-fields')
    +        ? $entity->toUrl('drupal:field-ui-fields')
    +        : Url::fromRoute("entity.{$bundle_of}.field_ui_fields", [$entity->getEntityTypeId() => $entity->id()]);
    

    If we to a if-else instead of a ternary, we can actually add a real @trigger_error() to the else branch for the deprecation. It's slightly more verbose, unfortunately, but I think it would be preferred in terms of the D8->D9 upgrade policy.

  4. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
    @@ -149,6 +149,7 @@ public static function getSkippedDeprecations() {
    +      'The "field_ui.deprecated_route_matcher" service relies on the deprecated "Drupal\field_ui\Routing\DeprecatedRouteMatcher" class. It should either be deprecated or its implementation upgraded.',
    

    I don't think we are allowed to add skipped deprecations at this point, so would be good to find some way to avoid that. Can we trigger the deprecation directly in Field UI's route subscriber maybe?

kristiaanvandeneynde’s picture

Re #60

  1. With this patch, the base path will be used to generate the actual routes. So we should check for the base path from now on. The base route will remain as an entity type key, but only so we know the route name we need to specify for the local tasks. So the base route depends on the base path being there, ergo we should check for the base path from now on.
  2. This is how core currently does it :) If you configure fields for nodes, you do it on the node type base route. Which kind of makes sense as you're configuring the fields for all nodes of a type, not for specific nodes. In absence of a bundle (such as the user entity type), core uses different routes (user account settings, for instance).
  3. We could change that, sure.
  4. Tests failed if I didn't add that, could you elaborate on this system and the approach you are suggesting? I've never seen this system before, but then again I was gone for months around the birth of my daughter.
tstoeckler’s picture

Re #61:

  1. But couldn't we look up the route name by the path when generating the local tasks? It would be unfortunate to have both the path and the route name in the annotation but if there's still runtime usage of the route name, then we can't really deprecate it.
  2. I'm pretty sure that's not true. I.e. I don't see any equivalent "switching" of the entity type in \Drupal\field_ui\Routing\RouteSubscriber::alterRoutes()
  3. Basically this means that if you add a deprecation you must also remove all usages of that code in core, otherwise the tests will fail. If it's not possibly to remove all usages (yet), then we cannot deprecate it (yet).
tim.plunkett’s picture

You can add the deprecation message to \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations() if it is not reasonable or possible to remove all the deprecations right away.

tstoeckler’s picture

Re #63 I'm pretty sure that's no longer allowed, but maybe a release manager can comment on this directly.

kristiaanvandeneynde’s picture

Re #62

  1. Maybe we could, but until we get a clear overview from #2999607: Fatal when trying to access systems you shouldn't, I'm afraid to make any code that builds something rely on other code that builds something. Although there is a case to be made that the local tasks already use the route provider, so maybe they can keep using it and we can deduct the route from there.

    One caveat: It's perfectly fine to share a path between multiple routes (provided you have route filters), so how would we know which route to select? By invoking the filters?

  2. You're right, we could just as easily declare them on the content entity (such as node) instead of the bundle entity. It comes down to preference, perhaps? I figured if you are configuring Field UI on the node type, then it would make most sense to have the link templates on the node type.

    Edit: Then again, looking at the changes to Entity.php, even I figured the templates would be on the base entity type. So either we keep that and set the templates on the base entity type. Or we keep setting them on the bundle entity type but remove the change from Entity.php

Re #64
Well, I'm introducing a deprecation here by marking the old way of doing things as deprecated. But to maintain BC, we can't immediately remove said deprecated code. So I figured it made sense to add this deprecation to the list of exceptions.

Off-topic:
While reviewing FieldUiLocalTask I noticed that in ::alterLocalTasks() it alters the tasks that it just generated in ::getDerivativeDefinitions(), only to add in the base_route key. Something it could have easily done while generating the tasks in the first place. Anyone got a clue why it's doing that?

tim.plunkett’s picture

kristiaanvandeneynde’s picture

Re #66 Okay cool, so how do I go about the following then?

+++ b/core/modules/field_ui/src/Routing/DeprecatedRouteMatcher.php
@@ -0,0 +1,55 @@
+/**
+ * Favors deprecated routes over correct ones for backwards compatibility.
+ *
+ * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Remove
+ * the old route names \Drupal\field_ui\Routing\RouteSubscriber in favor of the
+ * new ones when removing this class.
+ */
+class DeprecatedRouteMatcher implements FilterInterface {
+

I am introducing a service that no-one should ever use as its only intent is to maintain BC for the life-cycle of D8. It should be removed in D9, hence the deprecation marker. Which caused tests to go red unless I added it to the list of skipped deprecations.

andypost’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -318,7 +318,7 @@ protected function urlRouteParameters($rel) {
    -    if ($rel === 'add-form' && ($this->getEntityType()->hasKey('bundle'))) {
    +    if (($rel === 'add-form' || strpos('drupal:field-ui-', $rel) === 0) && ($this->getEntityType()->hasKey('bundle'))) {
           $parameter_name = $this->getEntityType()->getBundleEntityType() ?: $this->getEntityType()->getKey('bundle');
           $uri_route_parameters[$parameter_name] = $this->bundle();
    

    bleeding module's name to core is bad idea

  2. +++ b/core/modules/field_ui/src/Routing/DeprecatedRouteMatcher.php
    @@ -0,0 +1,55 @@
    +    if (count($collection)) {
    +      return $collection;
    

    no reason to count, just `if ($collection)`

kristiaanvandeneynde’s picture

#68:

  1. There is also mention of Field UI in the EntityType annotation and entity.api.php. I do agree it would be cleaner, though. But unless we check for field-ui, we need to move the link templates to the bundle entity type, as mentioned in #65.2
  2. Good catch, thanks

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.