Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Based on discussions at #2407505: [meta] Finalize the menu links (and other user-entered paths) system an 'entity:' scheme for a URI is needed to provide direct reference to entities for use e.g. in menu links.
Why entity:
and not entity://
was chosen is explained at #2407505-39: [meta] Finalize the menu links (and other user-entered paths) system.
Proposed resolution
Add logic to \Drupal\Core\Url::fromUri()
to also resolve entity:
URIs to Url objects.
Remaining tasks
Reviews and commit
User interface changes
None
API changes
New entity:
url scheme for directly referencing entities.
Beta phase evaluation
Issue category | Task because it is new functionality needed to resolve a critical issue |
---|---|
Issue priority | Critical because it blocks #2407505: [meta] Finalize the menu links (and other user-entered paths) system which is critical |
Unfrozen changes | Not unfrozen |
Disruption | API addition, disruption is minimal |
Comment | File | Size | Author |
---|---|---|---|
#91 | increment.txt | 4.79 KB | pwolanin |
#91 | 2411333-91.patch | 6.19 KB | pwolanin |
#87 | interdiff.txt | 3.61 KB | Wim Leers |
#87 | entity_uri_scheme-2411333-87.patch | 6.22 KB | Wim Leers |
#85 | interdiff.txt | 609 bytes | almaudoh |
Comments
Comment #1
almaudoh CreditAttribution: almaudoh commentedComment #2
pwolanin CreditAttribution: pwolanin commentedComment #3
pwolanin CreditAttribution: pwolanin commentedFrom dawehner, this should be trivial.
We will just resolve
entity://{entity_type}/{id}
to
Url::fromRoute('entity.{$entity_type}.canonical', [$entity_type => $id]);
Comment #4
BerdirJust want to point out that that won't work for config entities then, they don't have a canonical route.
Maybe the link template should at least optionally be supported as well? entity://node_type/article/edit-form ?
Comment #5
dawehnerSeems fair IMHO.
Comment #6
amateescu CreditAttribution: amateescu commentedSupporting
entity://view/frontpage
like suggested in #2407505-15: [meta] Finalize the menu links (and other user-entered paths) system might be tricky since that's not a link template, e.g. neither canonical nor edit-form.A possible answer is to support only content entity types with this
entity://
scheme since they usually have a canonical template, and then it will be as trivial as #3.Comment #7
almaudoh CreditAttribution: almaudoh commentedConceptually, I'm considering a new method
Url::fromEntityUri()
Comment #8
Wim Leers#6: did you see #4/#5? They discussed exactly that.
Comment #9
almaudoh CreditAttribution: almaudoh commentedTests to come...
Comment #10
almaudoh CreditAttribution: almaudoh commentedAdded test and fixed parsing code.
Comment #12
amateescu CreditAttribution: amateescu commented#8: Of course I saw that, my comment is a direct reply to them :) @Berdir asks if we should introduce link template support in the scheme and I say that it won't do any good since we still can't support the
view/frontpage
case.Comment #13
dawehnerWe can't support view/name anyway, given that we would need to support both display ID and arguments.
Comment #14
Wim LeersFixing nitpicks. Fixing tests.
I think we also want to validate the entity URI? That'd be the next step here.
#12+#13: gotcha.
Comment #15
almaudoh CreditAttribution: almaudoh commentedThanks @Wim. Here's some validation. Dunno if loading the entity here is efficient, but considering that in most cases the entity would already have been loaded for some other reason and therefore already in cache.
Also it means we can't use
Url::fromEntityUrl()
in any hook_entity_*_storage_load() hooks to avoid recursive loops.OTOH, are there use cases where we may want to link to an entity that doesn't yet exist?
Comment #16
Wim LeersLooks sane to me, thanks!
s/uri/URI/
Good question. I'm inclined to say "no", because how could you then possibly already know the entity ID to use?
Comment #17
jibranWhat if $entity_type is null or wrong? We should also add a check for that and can we rename it to $entity_type_id? Same in the @param description.
Comment #18
almaudoh CreditAttribution: almaudoh commented#16: Fixed
#17:
EntityManager already throws an exception for these cases so I didn't see any need to further handle it here.
We can, but the
getStorage()
signature still carries$entity_type
not$entity_type_id
For clarity, I also changed
$id
to$entity_id
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedIs loading the entity a performance concern at all? Maybe not given that we have the static cache and will probably need to do it anyway for access checking. So, if we're ok with loading the entity, how about instead of this, we just return
$entity->urlInfo('canonical')
?If we call urlInfo(), then we support any entity type that implements that properly. Looks like the 'view' entity type doesn't implement that, but perhaps we can open a separate issue for it to do so?
Could punt to a separate issue, but URIs support query strings, so we could extend the
entity://
syntax for Views (and other config entities that require arguments, like Panel Pages) to something likeentity://view/frontpage?display_id=page_1&arguments=...
or whatever else makes sense.+1. Either in this issue or in a follow-up would be fine with me.
What about making this protected, and having fromUri() call it if the scheme (which it already parses) is 'entity'? I could see us adding more internal schemes (as well as refactoring our spread-out logic of the 'base' scheme), and that way we don't need to expose each scheme as a separate public method to the outside world.
Comment #20
webchickAFAIK this blocks a critical and is thus critical.
Comment #21
pwolanin CreditAttribution: pwolanin commentedI think we should just handle the canonical route for content entities for a start - anything else seems problematic. I think we should resolve others from the path.
Comment #22
almaudoh CreditAttribution: almaudoh commented+1. Was thinking about this too. It simplifies usage (as it is now, an extra if statement plus parsing is needed to determine which variant of
fromXXUri()
to use). However this meansfromUri()
is re-purposed to accept any kind of uri as it was originally meant for non-drupal uris.Comment #23
larowlanNot to be difficult, but this hints of code smell.
Why does the Url object know this implementation detail? It seems like that is beyond its responsibilities.
The Url object is supposed to be a value object for detailing with Urls, are we sure we want it loading entities?
Comment #24
larowlanMoves the logic into fromUri and updates documentation.
Removes the loading, I think that is too much reach for a value object.
If we really want to validate the URL, we have processors for that right? And do we really want that? We don't stop someone from doing Url::fromRoute('entity.node.canonical', ['node' => 12]) if node/12 doesn't exist - how is this any different?
Similarly, what if someone stores this in a link field (using whatever widget we come up with) and then removes the node - do we want that to break that whole page from rendering? I wouldn't think so - I would think we'd want them to end up with a broken link - not a broken page.
Either way, if folks think loading and validating is what we want, I won't stand in the way.
Comment #25
larowlanprovided we do it in a processor ;)
Comment #26
Damien Tournoud CreditAttribution: Damien Tournoud commented^ Please use
parse_url()
properly instead of, erm, this thing.^ That needs a check on the number of exploded parts.
Comment #27
jibranI really like this apporoch.
Can we use strlen('entity://') instead of 9?
Comment #28
jibranWhat if I want to sore
entity://node/1/edit
?Comment #29
RavindraSingh CreditAttribution: RavindraSingh commentedThis is fine to use
+ $entity_path = substr($uri, 9);
entity:// = 9 chars, so the hardcoded 9 would work here.
I have also added t() for multilingual site security purpose.
throw new \InvalidArgumentException(String::format(t('The URI "@uri" is invalid. You must use a valid URI scheme. Use base:// for a path, e.g., to a Drupal file that needs the base path. Do not use this for internal paths controlled by Drupal.'), ['@uri' => $uri]));
Comment #33
RavindraSingh CreditAttribution: RavindraSingh commentedFormatted and cleand the patch.
Comment #34
RavindraSingh CreditAttribution: RavindraSingh commentedComment #35
dawehnerI really think we should ensure that the canonical link template exists, and otherwise throw a exception ... note: Url->toString() will throw one as well, but it will be simple less easy to understand.
Comment #36
xjmComment #37
hussainwebJust a little refactoring and addressing comments in #35 and partially in #26.
@Damien Tournoud (#26): I am not sure I like this handling of parse_url and check for entity as well, but can you suggest an alternative?
Comment #39
hussainwebFixing that embarrassing failure. :)
Comment #41
almaudoh CreditAttribution: almaudoh commentedLooks like we now have two different patches:
#33: no protected
fromEntityUri()
method#39: protected
fromEntityUri()
method still in place.Which are we going with?
The patch at #33 would be simpler and we can use the parse_url() method properly and not have to do any further parsing of the uri. E.g.
OTOH, the patch at #39 separates the entity parsing concern to a separate method as @effulgentsia mentioned in #19
which is easier for readability.
Comment #42
almaudoh CreditAttribution: almaudoh commentedOops, cross-post :)
Comment #43
hussainwebI don't think this is an acceptable solution, but hopefully, this will get it moving.
Comment #44
hussainweb@almaudoh: I reviewed the patches earlier and see that there was an
fromEntityUri
method which was subsequently removed. I think, however, it is not exactly the same. The problem with the earlier method was that it actually checked for the entity itself which could be considered a bit too much. Here, we are just checking if the canonical link template exists.Either way, I think it is still a good idea to move it to a different method. The relatively simple
fromUri
method suddenly started doing too much. For the record, I still don't like the condition to check for$scheme == 'entity'
in there but I understand we probably can't handleentity://
the same way we are handlingbase://
(as the former is just a shorthand for writing the proper route, whereas the latter actually points to a resource).Of course, I don't have enough experience to call the final shots on this, which is why we need reviews all around. :)
Comment #46
hussainwebA slightly different and better approach.
Comment #48
hussainwebFixing the failure, and also moving the test method near other tests.
Comment #49
almaudoh CreditAttribution: almaudoh commentedLooks good. Some comments...
This exception message should be updated to incorporate entity:// uris
I still think we could benefit from the parse_url() call already made in
fromUri()
, but I won't hang too much on it though.We should add a test for an invalid route.
Nit: Don't think this is needed since the next line initializes the
$map
array.Needs work for the invalid route test. But I will leave it for others to review too.
Comment #50
hussainwebAdding the test as per #49.3. Other responses:
1. I think it seems that this is more like an example. In any case, I don't think an exception message should be treated like documentation.
2. I kind of agree. I am wondering if it makes sense to make the new
fromEntityUri()
method as public and remove the functionality fromfromUri()
completely. That way, each of these methods will do only one thing and it will be clear on what it does. Right now it seems thatfromUri()
method might be doing a lot more than makes sense.4. Not really. The next line adds on to the
$map
array. We can, of course, combine both but I think it is better to leave it in separate lines.Comment #51
almaudoh CreditAttribution: almaudoh commentedPer #49.2, using
parse_url()
is cleaner, so here's an update for that.See #19 (last paragraph) and #22 for reasons why it was moved there in the first place.
Comment #52
almaudoh CreditAttribution: almaudoh commentedThe other thing with the route validation is that it only validates the entity type. It doesn't validate the actual entity.
Comment #53
hussainwebThis looks good. I was just trying to avoid another call to parse_url but of course, code clarity is better than micro-optimization, especially since this is not in any critical path as far as I can tell. :)
I see your points about fromUri() and fromEntityUri(). I did think of adding a method that would resolve the scheme and call the appropriate protected method (which is what fromUri() does). My main problem was that fromUri already did something else (handling unrouted URL's) and I did not want to change that. I think what we can do now is put in a new method handle unrouted URL's. That would be just a simple refactor and fromUri() would then be clean. Thoughts?
Comment #54
larowlanAnd I don't think it should,
Url::fromRoute('entity.node.canonical', ['node' => 12])
doesn't check if node 12 exists, so this shouldn't either. Loading and validating entities is not the realm of a Url object.Comment #55
larowlanAny reason not to use === here?
Should we use static instead of self? Someone might sub-class.
Comment #56
larowlanWas consideration given to using the ::toString() method (which does this exact check and throws the same Exception)?
Those three lines remove the call to ->getRoutesByName and therefore prevent the Url object from having to know the workings of another object (the route provider), instead deferring it to the UrlGenerator; which it already knows about and is clearly in the same domain (Urls).
Comment #57
almaudoh CreditAttribution: almaudoh commented+1. I think though that should be in another issue.
+1 - keeping it simple :)
Comment #58
larowlanWhat if someone passes
entity://node/1/edit
?$entity_id would end up being
1/edit
Note that the $url->toString() approach from my last comment should also validate that because the Route enforces a format on the entity ID slug.
Comment #59
Wim LeersFixed #55, implemented #56 (though that required a bit of mocking, which means we're not actually testing what we claim to be testing…).
#58: indeed, you'll end up with something like this:
[Mon Jan 26 08:32:59 2015] [error] [client 127.0.0.1] Uncaught PHP Exception Symfony\\Component\\Routing\\Exception\\InvalidParameterException: "Parameter "node" for route "/node/{node}" must match "[^/]++" ("100/edit" given) to generate a corresponding URL." at /Users/wim.leers/Work/drupal-tres/core/vendor/symfony/routing/Symfony/Component/Routing/Generator/UrlGenerator.php line 167
What I think is concerning is that
entity://node/such-epic-fail
happily renders to/node/such-epic-fail
. I can't believe this is intentional?This indicates one DX regression: previously,
Url::fromUri()
was for unrouted URLs. You'd always useUrl::fromRoute()
for routed URLs. That's no longer the case. I don't see a better way, but it's worth pointing out.Comment #60
dawehnerMH I would have checked for the canonical link template instead. Is there are reason to not do that?
... Yes, my comment still applies to the latest patch.
Comment #61
almaudoh CreditAttribution: almaudoh commented@dawehner the thing with using link templates is you have to load the entity first and we're trying to avoid that. The Url value object should not know about entities.
Comment #62
dawehnerThis is not TRUE. I was thinking about this:
Comment #63
almaudoh CreditAttribution: almaudoh commented@dawehner, I stand corrected :)
Some minor changes: added doc for @throws in fromEntityUri() and removed unused class variable in the test.
Comment #64
Wim LeersFix typo.
Comment #65
pwolanin CreditAttribution: pwolanin commentedTo what extent do we want to unit test? maybe
\Drupal::entityManager()
should be injected or at least allowed to be set?Is toString() the best way to validate a route exists? At the least, feels like that could be worth caching, or does the UrlGenerator at least have static caching?
Comment #66
larowlanI don't think Url objects should know about EntityManager objects.
$url->toString() will validate the route name and the parameter.
Not going to block progress because I realize this is critical to the menu link sprint, but hoping we can avoid coupling Url to entity manager.
Comment #67
Dave ReidCan we also have this store entity UUIDs instead of IDs?
Comment #68
hussainweb@larowlan: I think the patch in #59 did not use EntityManager but called the
toString()
method. Is that what you mean?@Dave Reid: Do you mean in the entity URI? Are you saying we should not use id's here but expect it to be an UUID? Our URI's would then be
entity://{entity_type}/{uuid}
.I suppose it makes sense to use uuid's as this path may be exported as config. The question still remains if we should support id's as well.
Comment #69
larowlan@hussainweb
This is still in the latest patch - that's the coupling I'm referring to
Comment #70
hussainweb@larowlan: True. But I was referring to this in #59:
Is this the implementation to which you were referring?
Personally, I don't like the above too much. Without the comment, it is not at all clear why we are calling
toString()
. Even worse, if the behavior oftoString()
changes later on, e.g., to check if the entity actually exists, this would indirectly affect this (I think it is okay for an entity:// style URI to refer to an entity that does not (yet) exist).On the other hand, I think coupling it with EntityManager is a bit too much out of it's job description. Between RouteProvider and EntityManager, I would pick RouteProvider as it makes some sense to the Url class at least.
Comment #71
larowlanUrl is already coupled to the UrlGenerator, could we just use that instead? We have #1965074: Add cache wrapper to the UrlGenerator for performance sake.
Comment #72
almaudoh CreditAttribution: almaudoh commented@larowlan we can't run away from the coupling with EntityManager after all it's an "entity" uri that's being resolved.
We could, however, delegate the entity uri resolution to the UrlGenerator. Is that what you're referring to?
Comment #73
Wim Leers+1
Otherwise menu links can potentially break when deployed.
Comment #74
dawehnerRegarding UUID support ... this is a dedicated issue we should discuss ... #2353611: Make it possible to link to an entity by UUID ... there are various approach you go use.
Well, I don't think that adding a coupling to the entity manager is any more coupling that adding support for entities in general.
In general I'm curious why
is protected ... if people are explicit, I would allow them to be so.
Comment #75
catchEntity reference fields store IDs, not uuids, so this is a general problem with content entities still.
If I create a node/1, how do I know what the uuid is to link to without checking in the database? entity://node/1 can be figured out from node/1 easily enough.
Comment #76
hussainweb@dawehner: Regarding fromEntityUri, it was discussed in #50 and #19 and #22 before that. It was not very conclusive except for keeping fromUri() as an entry point that can work with various types of URI's.
I still think it is a good idea to make this public (we need to add checks) and have fromUri() call this. Further, we can refactor rest of the functionality of fromUri() into another method. That way,
fromUri()
is purely a facade to handle different types of URI and if you know which one you need for sure, you can directly use the specific method.Comment #77
jibran@catch
For defautl values ER stores
target_uuid
please see EntityReferenceFieldItemList::processDefaultValue() and EntityReferenceFieldItemList::defaultValuesFormSubmit()Comment #78
Wim Leers#77: probably because that is stored in config, which requires you to use UUIDs. But the instances of a field are content, which has no such mandate.
Comment #79
pwolanin CreditAttribution: pwolanin commentedre-roll of #64 for conflict in UrlTest.php
Comment #80
pwolanin CreditAttribution: pwolanin commentedI agree with larowlan about the coupling - I think we should remove all the explicit validation in the new method and just let it fail at the time you try to call toString() I don't see what's gained by doing the work twice, other than potentially babysitting broken code.
If user input needs to be validated, this is not the right pace.
larolwan notes in IRC "we don't validate
Url::fromRoute('entity.node.canonical', ['node' => 12])
to see if 12 exists"So, the extra validation is inconsistent.
Comment #81
larowlanNot used
Not anymore
Not used either
Comment #82
larowlanOnly documentation changes and deletes, as per #81 - this is RTBC
Comment #83
jibranNW for IS update.
Shouldn't we be validating $entity_type_id?
Comment #84
almaudoh CreditAttribution: almaudoh commentedSee #80, this is similar to validating the route name in
Url::fromRoute()
and we don't do that.Should be "Thrown when the entity URI is invalid", we're not checking link templates anymore.
Comment #85
almaudoh CreditAttribution: almaudoh commentedUpdated issue summary and fixed small documentation nit in #84. Back to RTBC per #82 since the change was only a minor doc update.
Comment #86
almaudoh CreditAttribution: almaudoh commentedComment #87
Wim LeersYay, RTBC! Patch looks good.
I do still have one remaining concern: confusing DX.
Url::fromUri()
still says this:I think that's misleading to say the least. We added a paragraph after the last quoted one above for entity URIs, but that makes it pretty buried. So, on the surface, it seems one must only ever use
Url::fromUri()
when dealing with "external to Drupal" links. That's in fact specifically how this was designed: always use::fromRoute()
, and in exceptional cases, i.e. when linking to "external to Drupal" stuff, use::fromUri()
. With this change, that's no longer true, and hence this is now very confusing.That's also what I was getting at in #59. I think we should rephrase it to something clearer. Attached patch includes my suggestions.
And three nitpicks (all fixed in this reroll):
It's not resolving, but generating.
Weird code formatting.
Unnecessary whitespace change.
Comment #88
Wim LeersAnd one more thing: do we want
entity://
orentity:
? I.e.entity:node/1
orentity://node/1
? We're so used to seeing://
, but I'm not sure we need/want it here. Quoting pwolanin at #2412509-16: Change LinkItem.uri to type 'uri' rather than 'string' and introduce user-path: scheme:Comment #89
dawehnerI would have expected that PathValidator would understand
entity://...
but well, even people disagreed that we need support for that,we could at least still add support for it later.
Comment #90
pwolanin CreditAttribution: pwolanin commentedSo, in person discussion with Wim, xjm, dawehner, effulgentsia, mpdonadio, others - let's use
entity:
working on the patch.
Comment #91
pwolanin CreditAttribution: pwolanin commentedComment #92
pwolanin CreditAttribution: pwolanin commentedComment #93
xjmComment #94
dawehnerIt would be great explain in the issue summary why we went with that decision.
Comment #95
Wim LeersIt's explained at #2407505-39: [meta] Finalize the menu links (and other user-entered paths) system.
Comment #96
dawehnerIts fine for me.
Comment #97
xjmComment #98
webchickI "blind-reviewed" this on IRC since Drupal.org was down at the time. Questions/comments were:
- The docs mention base:// but I read in some other issue we're moving to base: ... however, this is fine since it's documenting how HEAD currently works and the issue that renames it will rename these docs as well.
- Lee raised some concerns about the coupling of Url to EntityManager, however this no longer happens in the current patch.
- The format is entity:node/1 but there are lots of known problems of linking to serial IDs versus UUIDs. But I see xjm has cross-linked the issue that would make it possible to link to entity:node/{uuid} instead. Cool.
Otherwise, this has docs, tests, and looks like what we talked about. Great work everyone! Onward! :)
Committed and pushed to 8.0.x. Thanks!