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.
Comment | File | Size | Author |
---|---|---|---|
#58 | interdiff-56-58.txt | 3.09 KB | kristiaanvandeneynde |
#58 | drupal-2651974-58.patch | 12.94 KB | kristiaanvandeneynde |
Comments
Comment #2
kristiaanvandeneyndeAttached 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.
Comment #4
kristiaanvandeneyndeForgot to update a method signature...
Comment #5
dawehnerMaking it public would be certainly kind of an API break, wouldn't it? Existing implementations would break.
Comment #7
kristiaanvandeneynde@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.
Comment #8
kristiaanvandeneyndeAttached 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().
Comment #9
dawehnerWell, i could actually imagine that contrib overrides that and then we have a problem.
Comment #10
kristiaanvandeneyndeSo a wrapper it is as suggested in #8?
Comment #11
dawehner@kristiaanvandeneynde How would you do that without any additional method?
Comment #12
kristiaanvandeneynde@dawehner: I couldn't. That's what I've been talking to myself about in #7 and #8 :)
Comment #13
kristiaanvandeneyndeHere's what I had in mind. No interdiff because it is a different approach than what I've posted before.
Comment #14
kristiaanvandeneyndeAlso, 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.
Comment #15
tim.plunkettWhy not just make the other method public?
EDIT: I see that's the difference between #8 and #13
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.
Comment #16
kristiaanvandeneyndeThanks 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.
Comment #17
kristiaanvandeneyndeComment #18
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedThe minor API change here would push this to 8.1. Also tagging for the Alexes to weigh in on that front.
Comment #19
kristiaanvandeneyndeI can live with 8.1.x, thanks for the review :)
Comment #20
dawehnergetUrlParameters is clearly not a classic getter, so maybe it should not be named like that.
Comment #21
kristiaanvandeneyndeI'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.
Comment #22
dawehnerRight, why do you not just make
urlParameters()
public and call it a day?Comment #23
kristiaanvandeneyndeBecause 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 ofgetRouteParameters()
. 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).
Comment #24
dawehnerOH, mh, sorry for the noise.
Comment #25
kristiaanvandeneyndeNo problem, it's Friday after all :)
So does this mean RTBC or does the architectural review need to come first?
Comment #26
tim.plunkettA fix for the config_translation module is blocked on this.
#2670712: ConfigEntityMapper::getBaseRouteParameters() should consult Entity::urlRouteParameters()
Comment #27
kristiaanvandeneynde8.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.
Comment #28
tim.plunkettYour 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
Comment #29
kristiaanvandeneyndeI 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.
Comment #30
kristiaanvandeneynde@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.
Comment #31
tim.plunkettThanks for clarifying :)
Comment #33
kristiaanvandeneyndeSo 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?
Comment #35
kristiaanvandeneyndeJust 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?
Comment #36
kristiaanvandeneyndeI 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:
drupal:field-ui-fields
Becomes:
entity.BUNDLE_ENTITY_TYPE_ID.field_ui_fields
.Actual route:
entity.BUNDLEABLE_ENTITY_TYPE_ID.field_ui_fields
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
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.Comment #39
grathbone CreditAttribution: grathbone commentedUnsure 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?
Comment #40
kristiaanvandeneyndeNot 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.
getRouteParameters()
elsewhere, so we'd be normalizing the name.So I see two solutions:
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.
Comment #42
kristiaanvandeneyndeOkay 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.
Comment #44
kristiaanvandeneyndeOh 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):
\Drupal\views\EventSubscriber\RouteSubscriber
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?
Comment #45
kristiaanvandeneyndeSeems 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.
Comment #46
kristiaanvandeneyndeUpdated the IS to reflect the new direction this issue has taken.
Comment #47
kristiaanvandeneyndeJust 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.
Comment #51
kristiaanvandeneyndeComment #52
kristiaanvandeneyndeThis 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.Comment #53
kristiaanvandeneyndeNo need for this any more. Will clean up after tests have finished running.
Should say Drupal 8.7.0
Comment #55
kristiaanvandeneyndeAre the tests failing because of the newly introduced deprecation markers?
Comment #56
kristiaanvandeneyndeLet's try this.
Comment #58
kristiaanvandeneyndeWhoops, forgot to keep using the old URL-generating logic for entity types without a Field UI base path.
Comment #59
kristiaanvandeneyndeCool, all green! Keep in mind that the new behavior might still need a few tests.
Comment #60
tstoecklerInteresting. 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?
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?
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.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?
Comment #61
kristiaanvandeneyndeRe #60
Comment #62
tstoecklerRe #61:
\Drupal\field_ui\Routing\RouteSubscriber::alterRoutes()
Comment #63
tim.plunkettYou 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.Comment #64
tstoecklerRe #63 I'm pretty sure that's no longer allowed, but maybe a release manager can comment on this directly.
Comment #65
kristiaanvandeneyndeRe #62
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?
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?
Comment #66
tim.plunkett@tstoeckler is 100% correct in #64. I opened #3002935: Document that DeprecationListenerTrait::getSkippedDeprecations() should not be expanded for new deprecations to clarify.
Comment #67
kristiaanvandeneyndeRe #66 Okay cool, so how do I go about the following then?
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.
Comment #68
andypostbleeding module's name to core is bad idea
no reason to count, just `if ($collection)`
Comment #69
kristiaanvandeneynde#68: