Updated: Comment #N
Problem/Motivation
Throughout core, we're working to use route name/parameter pairs in the place of a path:
- #2102777: Allow theme_links to use routes as well as href
- #2047619: Add a link generator service for route-based links
- #2056627: Form API autocomplete is broken for routes
Entities have a uri() method that returns an array of path and options, it needs to be replaced by route name, parameters, and options.
Proposed resolution
Convert Entity::uri() to return route name and parameters in an array
Add a Entity::url() helper:
// Before
$uri = $entity->uri();
$url = url($uri['path'], $uri['options']);
// After
$url = $entity->url();
Add a secondary method for code that wants the system path in isolation, without URL processing or querystrings:
// Before
$uri = $entity->uri();
$path = $uri['path'];
// After
$path = $entity->getSystemPath();
Stop manually appending things like '/edit' to paths, making it impossible to change them sanely:
// Before
$uri = $entity->uri();
$operations['edit'] = array(
'title' => t('Edit'),
'path' => $uri['path'] . '/edit',
'options' => $uri['options'],
);
// After
$operations['edit'] = array(
'title' => t('Edit'),
) + $entity->urlInfo('edit-form');
Remaining tasks
N/A
User interface changes
N/A
API changes
EntityInterface::uriPlaceholderReplacements() is renamed to urlRouteParameters() and no longer has to wrap the keys in '{' '}'.
EntityInterface::uri() no longer returns a 'path' key, but returns 'route_name' and 'route_parameters' and has been renamed to urlInfo().
Two new methods have been added, see proposed resolution section.
Comment | File | Size | Author |
---|---|---|---|
#44 | uri-2167641-44.patch | 228.71 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettThis is just a testing patch. Not ready for review yet.
Comment #3
tim.plunkettStill has a junk exception thrown.
Comment #4
tstoecklerWe're doing something similar in ConfigNamesMapper::getPathFromRoute() which I've never been very fond of. Would that be fixable in a similar fashion?
Might be a separate issue, don't know, but would be cool to know either way.
Comment #7
tim.plunkettI need to update the issue summary, but the failures are due to EntityInterface::uri() still using paths and not route names.
Marking #2165581: Get links from routes in EntityListController::getOperations as a dupe.
This patch needs a *ton* of work still, will get back to it tomorrow.
Comment #9
tim.plunkettStill need to update the issue summary, but for now here's a patch.
Comment #10
tim.plunkettThis $rel == canonical part is totally whack. Insider there $this->id() is always NULL, so the path is just 'entity/$entity_type', and is only used by CsrfTest... I have no idea what that's about.
This lets us remove almost every single EntityFormController::delete override method.
No more of this needed!
No more randomly appending strings to paths!
Comment #11
tim.plunkettComment #12
tim.plunkettComment #13
tim.plunkettForgot to include the getInternalPath() part.
Comment #14
tim.plunkettForgot to fix ImageFieldDisplayTest.
Comment #17
tim.plunkettMissed a spot!
Also cleaned up a couple docblocks.
Comment #18
tim.plunkettI realized the weird hack I left in the code for REST stuff was no longer needed. Removed that.
This now obsoletes #2107581: Configurables uri() always returns 'edit-form' link, because it contains the fix by necessity, and I've added the test coverage from that issue, and expanded it.
Leaving that open for now though.
This is now ready for full review.
Comment #19
tim.plunkettComment #20
tim.plunkettComment #21
tim.plunkettAfter a long discussion with @sun and @Crell, I've renamed the methods.
uri() is now urlInfo().
url() stays.
getInternalPath() is now getSystemPath(), to mimic the naming of AliasManagerInterface and _system_path
Comment #22
tim.plunkettRerolled after #2168333: Ensure custom EntityType controllers can be provided by modules
Comment #24
tim.plunkett22: entity-uri-2167641-22.patch queued for re-testing.
Comment #26
tim.plunkett22: entity-uri-2167641-22.patch queued for re-testing.
Comment #27
Crell CreditAttribution: Crell commentedClassy.
How will #2010184: [meta] convert ‘uri_callback’ entities param to EntityInterface::uri() method impact this? The url callback function is problematic anyway at this point.
Is there a reason to not todo this while we're already modifying this code? What is this todo blocked on?
urlInfo() has the same check, but throws an exception. This returns empty string. If there's a reason for them to be different then it should be documented.
Please don't delete this method. It will be needed once we move back to URI templates on entities. (I know you disagree with that; that discussion belongs in the other issue, not here. Leave it for now, please, so we don't have to deal with that here.)
The distinction between these two methods is not 100% clear. url() also includes aliasing, for instance.
perhaps url() should be listed as "Returns the public URL for this entity", vis, the one that gets displayed to the world. Then note the lack of aliasing in getSystemPath().
Also, I don't think we've referred to other methods in the same class as self::whatever() before in documentation. I'm not against it, but I haven't seen it as a convention yet.
This is so much closer to the world I'm looking for. :-) (Still not 100% there, but way closer.)
While I'd still prefer these were URI templates, I like that the links array is getting more widely used with more entities.
I didn't go over the rest of the patch in detail, as it's just applying the above patterns a lot.
Comment #28
tim.plunkett2) Well that issue is about completely deleting this code, so it doesn't matter that I move it. (It looks like I'm changing it, but if you apply the patch and use -w you'll see I'm mostly outdenting)
3) It just wasn't in scope, I didn't want to add test coverage, and #2010184: [meta] convert ‘uri_callback’ entities param to EntityInterface::uri() method plans to delete it anyway. I didn't know it was, but I like that. This patch should actually make that issue easier.
4) Will add docs, but returning an empty string meets the expectation of the callers.
5) I'm not going to just leave dead code around. This is a self-contained and simple method that can be readded if we decide to. And I didn't just delete it, it was replaced by urlRouteParameters(), which is essentially the same thing without the wrapping {...}
6) I'll make that more explicit. And yes, we do use self:: for inline docs now.
Comment #31
tim.plunkettHeh, the hook_menu_links_default patch also added a vocabularyTitle title callback, just to different parts of the same file :)
Comment #32
tim.plunkett#1987396: Refactor & Clean-up comment.module theme functions moved a bunch of code I was changing, but it was easy enough to fix...
#2181045: Add a title callback for the term overview page went in to prevent further clashes with the hook_menu_link_defaults issue.
Comment #33
dawehnerFirst bit before the browser stopped to work.
Should we add a follow up to not provide some default? It feels like of wrong to do this.
This seems to be something new, do we need this?
Can we also explain here why this is expected?
It is a bit odd to hardcode that, but I guess there aren't any better fix.
Is this required as part of the patch?
Comment #34
Crell CreditAttribution: Crell commented#33.1: That's actually quite smart. The base class has a default of "canonical". For config entities, though, there is no "view" page. I suppose technically we could make canonical and edit-form the same URI, but in most cases if you want "the link I should click to get to the config entity", you probably mean the edit-form. So we either do this, or set edit-form and canonical to be the same path. I suppose I could be happy with either. It's already doing the edit-form version in HEAD right now, though.
Comment #35
tim.plunkett1) I think we should either remove the default everywhere or leave it here. Entity has a default of 'canonical', but config entities don't have a canonical link template...
2) We absolutely need this now. Previously, we would just manually build up a path, and you would end up with
'entity/' . $this->entityType
for non-saved entities. That doesn't work now with the route-based approach.3) It is expected by the calling code we have in core. They just want to know what string to use, and an empty string is fine. Forcing them to do the isNew check, or worse catching the exception, is not expected. Unlike when calling urlInfo() yourself, when you need to introspect the actual route name and params.
4) Yeah, there wasn't much else to do here.
5) I was going through and fixing each entity form's delete(), until I realized they were all the same when there was a delete-form link template.
Forgetting to write this delete() method has burned me on every ConfigEntity I've written, so I'm very happy about this being in.
Comment #36
tim.plunkettThis conflicted with:
#2100183: Remove the changed() method from EntityInterface
#2172973: Ensure ConfigEntities providing a list controller use admin_permission
#2110467: Add first(), get($index) and possibly other methods to ListInterface
Thankfully none of the actual lines were changed, just context conflicts.
Comment #37
tim.plunkettConflicted with #2164367: Rebuild router as few times as possible per request on patch context.
EDIT: #2182899: Use the --3way option with git apply would be a way to prevent some of these rerolls.
Comment #38
BerdirLooks great to me overall. Two warnings:
- I did not read anything in this issue at all, I just read through the patch. If anything was discussed already, feel free to point me to the discussion or ignore it :)
- Lots of small stuff below, but I did went through the whole patch and wrote down my thoughts. Sometimes I ask questions and then answer them myself later on, so read everything before you respond :)
Thank you! I was trying to suggest that in issue that changed that but it was ignored ;)
I think entity_type was removed explicitly because you can access it through 'entity']->getEntityTypeId.
This and other parts will also conflict with the getEntityTypeId() issue.
I know there was an issue with the idea to enforce that entities always have a URL, that's where the default entity/ stuff was introduced. No longer possible with routes obviously, wondering if that could be a problem somewhere, e.g. for rest or so?
Not sure, but the : makes it sound like those two are the only options, is that necessary?
Not sure if I understand what this is checking exactly, a comment might help?
Ỳay, more clean-up of only partially supporting the $rel argument :)
Not sure, but just "self::url() will return.. " sounds better to me.
Is the mix of system path and #fragement for a destination URL really correct?
Mixing system path and urlInfo() options seems a bit strange, are you sure this is compatible?
This is nice, so every entity has a link template for this.
Just wondering, all link templates are I think right now added to the rest output of an entity, do we really want all that stuff there?
Can see that change repeated below, why does that change here?
It's a good thing that we wrote quite a few tests for all these links as we broke it multiple times already.
Might be useful to go through all entity lists manually, if this didn't happen already?
The default implementation renmoves the destination after using it, this doesn't, why?
Ah, I think I know what's going on, overview was the default tab before and now it's edit?
I don't get why, though?
Just wondering if this is still necessary after the router rebuild stuff went in?
Unlike one of the examples above, this does not add the options, which were there before...
I know, out of context, just wondering, but this should be handled through the access controller, not by altering the operations..
Makes sense, just wondering why it's necessary in the context of this issue? Given that url() returns '' for new entities anyway.
Not a pattern that you are introducing here, but enable/disable are just as form-y as delete, no?
A follow-up to use the same pattern for translate routes as we do for field ui would be nice?
Yep, as I guess above.
Are the title changes necessary for tests?
And still wondering why the default tabs are switched for vocabulary and I think shortcut set above was similar?
As you add a type hint, shouldn't you make that a UserInterface while you're at it?
Comment #39
tim.plunkett1) You're welcome :)
2) In HEAD the entity_type is still added in only part of the code path. I didn't want to remove it here.
3) #2019123: Use the same canonical URI paths as for HTML routes is addressing this
4) That's copy/pasted.
5) Added a comment. This is for getting to /admin/structure/types/manage/article for a node entity.
7) Fixed
8) and 9) This is absolutely not okay, but it is done in HEAD, and very out of scope here. This is one of the few offenders outside of the translation modules and Views.
10) I'm not sure if we want that, but we're not changing anything, just expanding the usage of it. Might be worth a REST follow-up.
11) So all config entities with a UI have an edit-form. Except in order to trick the UI, vocabulary defines its overview page as its edit form, which is just not true. In order to correctly identify the correct edit-form while maintaining the expected taxonomy UI, we need to massage these paths.
12) I manually went through each entity type myself. More had test coverage than I thought, which was nice.
13) I believe this was just an oversight on my part. If something fails, we'll know why and can add a comment.
14) See point 11. This is the opposite of adding /overview/ to places.
15) Yes this is still necessary. That issue did not help DUTB tests at all, because nothing ever sets the "if needed" flag.
16) Turns out I had half of the right code in PictureFormatter, and the other in ImageFormatter. Fixed.
17) It would be nice...
18) This shouldn't be needed anymore, it was before some refactoring to avoid the exception in urlInfo(). Fixed here and in EntityNormalizer.
19) No, these (and Views UI) just do the enabling/disabling and redirect.
20) See points 8/9
21) The title change is needed to maintain the expected UI and breadcrumbs
22) Sure!
Comment #40
tim.plunkett#2167109: Remove Variable subsystem changed the old EntityUriTest web test, which is being removed here.
Comment #41
tim.plunkett40: uri-2167641-40.patch queued for re-testing.
Comment #42
tim.plunkettSomeone pointed out that we shouldn't have a non-critical "Avoid commit conflicts".
If we don't finish the conversion from paths to route names before release, all of that time was wasted. Route names are useless if anything in code refers to a path.
Comment #43
tim.plunkettReroll for #2177739: Fix inefficient config factory caching
Comment #44
tim.plunkettI knew that working on #2164827: Rename the entityInfo() and entityType() methods on EntityInterface and EntityStorageControllerInterface would bite me.
Comment #45
Crell CreditAttribution: Crell commentedLet's do this.
Comment #46
alexpottCommitted 76bcb27 and pushed to 8.x. Thanks!
Comment #47
alexpottWhilst reviewing this I noticed #2186225: Fix documentation of Drupal\Core\Entity\EntityInterface::urlInfo()
Comment #48
xjmComment #49
aspilicious CreditAttribution: aspilicious commentedI think these examples should be routified now?
Probably needs a small followup for this...
Comment #50
XanoYup, that's what Alex created #2186225: Fix documentation of Drupal\Core\Entity\EntityInterface::urlInfo() for.
Comment #51
tim.plunkettOpened #2188915: Stop appending paths to Entity::getSystemPath() to address #38.9
Comment #52
ann b CreditAttribution: ann b commentedHi,
I'm going to attempt to write the change record for this issue. I am fairly new to drupal though. If this change record should really be handled by a senior developer, just let me know. I'll change the status back to unassigned.
Ann
Comment #53
ann b CreditAttribution: ann b commentedI have created a change record. Let's see how it goes. Please see https://drupal.org/node/2221879.
Comment #54
tim.plunkettSince #2215961: Entity::urlInfo() should return \Drupal\Core\Url went in, all of this changed. I used @ann b's work as a base, can someone review?
Comment #55
tim.plunkettThere is a draft, but its "missing"
Comment #56
xjmComment #57
pwolanin CreditAttribution: pwolanin commentedThe change record seems to be published? It looks fine, so marking this fixed.
Comment #60
xjmActually it was critical; I was just confused by the CR noise.