Updated: Comment #31
Problem/Motivation
The ContentTranslationControllerInterface::get*Path()
methods, which provide the paths to be used when building the UI for a particular entity type, should not be translation specific. AAMOF this is a common need and we should be exploiting a generic API instead of providing a one-off solution.
Proposed resolution
Rely on the Entity links API introduced in #1970360: Entities should define URI templates and standard links to generate entity URLs in a generic fashion. Remove the code implementing the current Content-Translation-specific API, deprecate what cannot be removed yet.
Remaining tasks
Evaluate what has come up from #1783964: Allow entity types to provide menu items and #1332058: Need a way to distinguish "public/final" URIs for entities from admin and redirected ones.Propose a solution.Post a patch to implement it.Do reviews, update or write tests, evaluate if documentation or a change notice is needed.
User interface changes
None
API changes
- Removed the
menu_view_path
,menu_edit_path
andmenu_path_wildcard
entity keys. - Deprecated the
menu_base_path
entity key (removal needs to wait for the conversion to the new routing system to be complete). - Added the
links
entity key definition to the custom block entity type. - Removed the
getBasePath()
,getEditPath()
andgetViewPath()
methods from theContentTranslationControllerInterface
.
Background
Originally in #1188388-19: Entity translation UI in core
plach
The get*Path() methods, which provide the paths to be used when building the UI for a particular entity type, should belong to a UI controller or whatever comes up from #1783964: Allow entity types to provide menu items and #1781372: Add an API for listing (configuration) entities where the same issues we have here are being addressed. AAMOF those methods are not specific to translation and atm we need to inject menu router info in the entity info to be able to generate the translation UI in a generic fashion.
And responded in support in #1188388-81: Entity translation UI in core
Gabor
There was a lot of discussion on the entity listing patch about paths known to the controllers, and the need for getURI if only admin paths are known, etc. I think these are fine to discuss in the mentioned related issues and we can keep refining this without the need for any major rework, making this patch free to use its current ways.
Comment | File | Size | Author |
---|---|---|---|
#82 | ct-paths-1810350-82.patch | 47.05 KB | plach |
Comments
Comment #1
plachComment #2
plachComment #3
Gábor HojtsyComment #4
PanchoComment #4.0
PanchoUpdated issue summary.
Comment #5
plachI will provide an inital patch for this.
Comment #6
plachWe can now rely on #1970360: Entities should define URI templates and standard links to fix this.
Related: #1968970: Standardize module-provided entity info documentation and clean-up the @EntityType annotation.
Comment #7
plachComment #8
plachHere is an initial patch. The
menu_base_path
is retained for now as we need full conversion to the new routing system to remove it.Comment #10
kfritscheTrying to get some fails down here now.
Attached rerolled patch.
Comment #11
kfritsche--duplicate--
Comment #12
plachThanks!
Comment #13
kfritscheOkay took me a while but this patch should replaced all the rest of getBasePath, getEditPath, getViewPath. Was used in some tests and some other functions.
Also the custom block didn't used it at all.
Further added links to the annotations in the EntityTest Modules so that this is working correctly.
All test which failed should go green now. Hopefully nothing new breaks ;)
Comment #15
kfritscheFixed the access problem in comments.
Added a new Test Entity for the URI so this doesn't conflicts with the translation things.
Now it should go green.
Comment #16
plachAwesome work really! We are just missing these minor things below and then we are good :)
Just
$entity->uri()
should be enough.Debug leftover :)
Comment #17
kfritscheRerolled because of #1754246: Languages should be configuration entities got in.
Changed these two minor things mentioned by plach in #16.
Comment #18
plachCool!
RTBC if green :)
Comment #19
plachRemoved some more obsolete docs, and marked what cannot be removed yet as deprecated. Functionally code is the same.
Comment #20
BerdirDoc improvements look good to me, so keeping this at RTBC is fine.
Comment #21
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #22
plachChasing head.
Comment #23
plachAgain
Comment #24
Gábor Hojtsy#23: ct-paths-1810350-23.patch queued for re-testing.
Comment #26
plach#23: ct-paths-1810350-23.patch queued for re-testing.
Comment #28
plachIt was just a copy/paste problem: the new no-URI test entity-type had the wrong label. Setting back to RTBC as this was definitely minor.
Comment #29
Gábor HojtsyMake sure proposed API changes have the API change tag.
Comment #30
alexpottQuoting from the current issue summary...
... seems like we have a solution... let's get an update :)
Comment #30.0
alexpottEntityTranslationControllerInterface was renamed to ContentTranslationControllerInterface
Comment #31
plachUpdated the issue summary, sorry for missing that. I hope it's ok to set this back to RTBC, anyone feel free to change the status if not.
Comment #31.0
plachUpdated issue summary
Comment #32
alexpottThe edit-form is actually block/{id} too ... see #1983682: Convert applicable custom_block_menu() entries to use _entity_form in routing.yml
Comment #33
plach...and fixed :)
Comment #34
alexpottI think $base_path is not a great variable name because the entity annotations still have a base path key. Perhaps $translation_path is better?
Should be EntityTestNoUri
Needs a new line after */
Comment #35
plachAddressed #34.
Comment #36
alexpottOh no! I've made it worse :( sorry plach... if we need this variable perhaps $base_translation_path?
However reviewing the code...
The
if ($translation_path) {
looks completely unnecessary as it is not possible for en entity not to have a translation-overview link. So I think this code can be simplified quite a bit...Comment #37
plachThis should look better now :)
Comment #38
webchickSeems like Alex has been all over this one. :)
Comment #39
plachRerolled
Comment #40
plachRerolled again
Comment #42
plachBetter reroll
Comment #43
plachPromoting to major as this is heavily touching the Content Translation module API: the more we wait for this to be committed the higher is the likelihood that some module out here will break.
Comment #44
alexpottWe shouldn't be deprecating stuff and still using it in core. So we either need to open a followup to remove of do it here. I'm favour of the latter as it gets this closer to done.
Comment #45
plachOk, removed the
menu_base_path
key altogether. The current solution is a bit unreliable but it just has to work until we are done with the new routing system conversion.Comment #47
Gábor HojtsyNot passing :(
Comment #48
plach#45: ct-paths-1810350-45.patch.patch queued for re-testing.
Comment #49
plach@Gabor:
Passing :)
Comment #50
plachAs discussed with @alexpott in IRC, I added a couple of @todos linking the follow-up issues which will allow to get rid of the current entity link/router path futzing, namely #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit and #1987882: Convert content_translation routes to a new style controller.
Comment #51
plachMore accurate title.
Comment #52
plachI no longer can RTBC this one. Anyone available for a final review? Gabor?
Comment #53
Gábor HojtsyLooks like all changes requested by @alexpott have indeed been taken care of well.
Comment #54
catchThis looks a bit like the sort of change I'd expect to see in #2040379: Define contextual entity operations through annotation; remove the context property from hook_menu items. I don't know if it complements or conflicts with that one though. Note in that issue there was discussion of using routes rather than paths.
Comment #55
pwolanin CreditAttribution: pwolanin commented@catch, yeah, the way these link are defined is still very problematic. Ideally we'd define all the entity routes based on these patterns, rather than duplicating them in the annotation and the route
Honestly, the pattern alone isn't something we have good code in place to handle, so I'd like to see something like the entity methods returning a route name and parameters instead of a path, possibly based on the content type (e.g. html vs JSON). This patch is probably a step in the right direction, but it will need follow-ups.
w.r.t. #2040379: Define contextual entity operations through annotation; remove the context property from hook_menu items another option there might be to make them plugins with automatic per-entity-type derivatives? Just haven't had time to get traction on that issue - I'm afraid it's likely to be a late API change.
Comment #56
plach@catch:
Mmh, I think that one would provide an even cleaner way to address this issue, AAMOF what we are doing here is defining a
'translation-overview'
link, which makes much more sense as an operation rather than a relationship link. I am going to ask feedback in the other issue, hence moving back to NR until I get an answer.Rerolling meanwhile.
Comment #57
plachRerolled.
I am setting this back to RTBC as #2040379: Define contextual entity operations through annotation; remove the context property from hook_menu items is not moving atm. As pointed out by peter this may need a follow-up but at least we would be removing a deprecated API that has stayed in D8 core long enough.
Comment #59
kfritscheI'm trying to get this green again.
Comment #60
kfritscheIn the ContentTranslationRouterSubscriber was still a use of menu_path_wildcard and menu_base_path.
This were replaced and the tests should work now.
Also manually tested if the translation of a node and a custom block is working.
@Plach: Feel free to review it again and set it to RTCB, if you are fine with my change. (see interdiff)
Comment #61
kfritscheAnd an update. There was still something I tested in #59, why the delete translation tests will fail.
Here the update for it.
Comment #62
plachThe change looks the right thing to do, but it's very unfortunate we have to do all this path futzing. That clearly indicates there's still something wrong with our APIs. I really hope we can exploit #2040379: Define contextual entity operations through annotation; remove the context property from hook_menu items to solve this but I'm not actually sure.
I will try to have someone with more knowledge than me on the route/menu stuff to have a look to the patch to get some suggestions.
Comment #63
boran CreditAttribution: boran commentedHad a read through the mods and found no logical mistakes - the changes are consistent throughout the affected files.
As regards doc, a nit pick would be to keep the present tense (we dont need to converts to past tense because something is deprecated) and follow the comment guidelines more closely (https://drupal.org/node/1354).
Comment #64
boran CreditAttribution: boran commentedUsed this as an excuse to learn to create an interdiff :-)
Comment #65
YesCT CreditAttribution: YesCT commented@boran :) Let's work on this again tomorrow.
The interdiff is backwards
in comments name spaces should start with \ (an example in https://drupal.org/node/1354#file, and the standard in https://drupal.org/node/1353118 )
and....
we need the whole patch too. :)
Comment #66
boran CreditAttribution: boran commented@YesCT:
- name space comment corrected.
- updated interdiff and patch
Comment #67
kfritscheDid a last review of the patch and manual testing. Only some nit picking left, which I fixed.
Added a comment to the str_replace in the ConfigTranslationRouteController to explain why are we replacing the entity_type name with just {entity}.
Also noticed that in the entity_test and custom block entity the canonical link has no trailing slash, but all other canonical links in node, comment, term and user have a canonical link. Changed this too for consistency.
Added curly brackets around the brackets. With only one curly brackets PHP thinks this is the Complex (curly) syntax and just adds $entity_type again, without the curly brackets around.
Manual testing:
* Clean install in german with patch
* Enabled content translation
* Added english as second language
* Enabling translation for article
* Created a article
* Translation overview: node/1/translations
* Added translation: node/1/translations/add/de/en
* Edited translation: node/1/translations/edit/en
* Deleted translation: node/1/translations/delete/en
All urls are reachable and are working.
As I worked on this issue before I'm not sure if I should RTBC it.
Comment #68
vasi1186 CreditAttribution: vasi1186 commentedThe code looks good to me, found nothing that should stop this to rtbc, from the code style point of view and implementation. But probably needs a review also from some people that were more involved in this issue before :)
Comment #69
plachI'd still like to get some expert's feedback on the latest changes. I'll try to get @fubhy have a look to this.
Comment #70
Gábor HojtsyI asked fubhy (again) on IRC to post his feedback here.
Comment #70.0
Gábor HojtsyUpdated issue summary.
Comment #71
tim.plunkett67: ct-paths-1810350-67.patch queued for re-testing.
Comment #73
plachRerolled, performed some additional clean-up and added a proper namespace to the new CT relationships. Hopefully we should good to go now for reals.
Comment #74
plachTo do: this can go away now.
Comment #76
plachFixed local tasks test.
Comment #77
plachGiven that #2040379: Define contextual entity operations through annotation; remove the context property from hook_menu items was closed in favor of a solution dealing with contextual links and not operations, I think the way forward here is the current one. We should be ready for commit, reviews for the latest changes are welcome.
Comment #78
Gábor HojtsyOnly two things stood out to me:
Should this not be removed, or would that be a different issue? It is not related to this issue scope BTW, so while I see the urge to document it, seeing it like it is is puzzling :D
HUGE DX WIN!
So why do we need the node/%node old type path? This type of path is archaic at best in the new system. I don't see anything left in this patch that would handle these style paths?!
Comment #79
plachWe have #1968970: Standardize module-provided entity info documentation and clean-up the @EntityType annotation to complete the clean up.
Yeah, I know, but this was written just before the "code freeze" of the first of july. Do you think we should just remove it in the issue above?
We still have
content_translation_menu()
andcontent_translation_menu_alter()
with the related @todos. I removed the call incontent_translation_entity_info_alter()
though, plus another clean-up.Comment #81
Gábor HojtsyYeah I think the first hunk should be delegated to #1968970: Standardize module-provided entity info documentation and clean-up the @EntityType annotation, not in the scope for this.
As for the old style paths, I see the hook_menu() relation now and conversion to route subscriber should fix that. There are already issues for that, and this is just a stop-gap. So I think that is fine then.
Comment #82
plachOk, rerolled and removed that hunk
Comment #83
Gábor HojtsyLooks great to me now. Unifies DX nicely, no special properties only used by content translation. Fabulous!
Comment #84
catchThis is great. Committed/pushed to 8.x, thanks!
Will need a change notice update I think.
Comment #85
Gábor HojtsyI've been trying to find an existing change notice to update, but not sure which one would be sufficient... :/
Comment #86
plachMaybe in https://drupal.org/node/2028009?
Comment #87
Gábor HojtsyI don't think that is relevant, it does not talk about any annotation stuff :) Also I found https://drupal.org/node/1827470 but no mention of content translation annotations there either.
Created this new change notice: https://drupal.org/node/2133913