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.
Part of #1971384: [META] Convert page callbacks to controllers
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Comments
Comment #1
acrollet CreditAttribution: acrollet commentedComment #2
acrollet CreditAttribution: acrollet commentedI'm working on this issue.
Comment #3
acrollet CreditAttribution: acrollet commentedI need to leave town soon and didn't get as far as I would have liked, but I'm uploading some partial work as a patch. Because dynamic routes are generated for entity types with translation enabled, a routing yml file can not be used, and a RouteSubscriber interface has to be implemented. This patch adds the RouteSubscriber service and creates routes for the translation overviews page, but the controller still needs to be implemented.
Comment #4
InternetDevels CreditAttribution: InternetDevels commentedPrevious patch incorrectly used wildcards in routes. We've added converters to routes definition and moved translation_entity_overview() code to the separate controller class.
Translation overview page (/{entity_type}/{entity_id}/translations) works fine with attached patch.
Comment #6
InternetDevels CreditAttribution: InternetDevels commentedPrevious patch incorrectly generated routes (incorrect route path and access rules).
Also we've added new class for control access to entity translation overview page .
Translation overview page works fine with attached patch.
Tested with all entities that exists in core.
Comment #7
andypostCode looks good, just needs manual testing
Comment #8
dawehnerThanks for working on this conversion!
Should be "Contains \"
Controllers should be in the Controller directory.
Parameters should be documented and contain the full namespace in the documentation.
You could just use $this->entityManager->getControllerClass($entity->entityType(), 'translation');
module_exists can be replaced by $this->moduleHandler->moduleExists()
Is there a reason why you remove the context?
Comment #9
InternetDevels CreditAttribution: InternetDevels commentedThanks for the review!
I've fixed almost all Your notices, but should I really move EntityTranslationOverviewAccess.php from Access to Controller folder? Currently I've kept it in the Access folder. Patch attached.
Comment #10
andypostFiles' structure is ok.
Comment #11
InternetDevels CreditAttribution: InternetDevels commentedAttached patch also covers #1987878: Convert content_translation_edit_page() to a new style controller and #1987876: Convert content_translation_add_page() to a new style controller issues, but needs some additional work to get rid of the hard dependencies in the code.
I will work with this all in few days.
Comment #12
star-szr@InternetDevels - Thanks for working on this! I'm wondering why those other patches got combined here though. I think if possible these patches should be separated into their own issues so they can be reviewed individually.
Comment #13
vijaycs85Blocked by #2024867: Rename translation_entity to content_translation
Comment #14
Gábor HojtsyThe other one is actually easier to reroll(?)
Comment #15
dawehnerShould be "Contains \..."
Wouln't it be possible to merge these two together .. .the code looks really similar in most places.
This could be just @inheritdoc.
Please return static::ALLOW and static::DENY
This could be replaced by a controller coming from entityManager. I suggest a method called getControllerInstance($entity_type, $controller_type); which works like every other getAccessController like method.
See above.
Same as before.
Should be better "Constructs a EntityTranslationOverviewController object." (this is the standard.
This code then could leverage the new method from above.
Type hinting should contain the full namespace as well.
entity_get_form now has a method on the entity manager.
Should both apply or just one of them?
Here should be a new line, sorry.
I guess you can't remove the 'title' now? already as well as the type@
Comment #16
vijaycs85Fixed all in #15 except below one:
This code then could leverage the new method from above.
and updated controller & access classes.
Comment #17
vijaycs85Interdiff after conflict merge....
Comment #19
vijaycs85As discussed with @dawehner on IRC, updating entityManager to get the controller directly.
Comment #21
vijaycs85Hopefully this patch fixes few test cases...
Comment #23
Gábor HojtsyComment #24
tim.plunkettThis is a straight reroll, but a lot of this doesn't make much sense to me.
For one, the RouteSubscriber is written very differently from the others I've seen in core.
Second, the access checker should use StaticAccessCheckInterface instead, and can have injection done via services.yml, I don't think those work with ControllerInterface.
Thirdly, the change to EntityManager::getController() shouldn't be needed, but even if it is, the calls to it make no sense.
Comment #26
tim.plunkett#24: content_translation-1987882-24.patch queued for re-testing.
Comment #28
jibran{inheritdoc}
$account->hasPermission()
We can use https://drupal.org/node/2019739 now and remove @todo
Also adding to @Crell's list to get a better review.
Comment #29
vijaycs85Updating review changes in #28
Comment #30
vijaycs85restoring tag.
Comment #32
jibranThanks @vijaycs85 for fixing the issue one minor thing.
Please restore this change. I talked to @tim.plunkett and he explained to me that we are not using
'#type' => 'html_tag'
anywhere other then<head>
in the core so it is not alright to use it here as it is slow and not made for this.Comment #33
jibranVendor namespace should be in the end.
I think we should use
static::ALLOW
andstatic::DENY
We can get $account from
$request
and replaceuser_access
withhasPermission
This should be in manger or module file. So that we can remove
$this->moduleHandler->loadInclude('content_translation', 'inc', 'content_translation.pages');
then we don't need to injectmoduleHandler
.field.info
service injection$path
is not required.Not required here.
Comment #34
Gábor HojtsyThe longer we stretch these out the more code changes in the removed/moved code and more potential for error. Is anybody working on this one?
Comment #35
vijaycs85@Gábor Hojtsy, trying to give a try :)
Thanks for the confirmation @jibran, rolled back #32.
On #33:
#33.1 - FIXED: Vendor namespace should be in the end.
#33.2 - FIXED: I think we should use static::ALLOW and static::DENY
#33.3 - FIXED: We can get $account from $request and replace user_access with hasPermission
#33.4 - NOT FIXED: This should be in manger or module file. So that we can remove $this->moduleHandler->loadInclude('content_translation', 'inc', 'content_translation.pages'); then we don't need to inject moduleHandler. - Not sure why would we need to do this here now. Can't we clean up on follow up?
#33.5 - FIXED: field.info service injection
#33.6 - NOT FIXED: $path is not required. - as per #33.4, we aren't fixing it here.
#33.7 - NOT FIXED: Not required here. - as per #33.4, we aren't fixing it here.
Comment #37
Crell CreditAttribution: Crell commentedBy convention, most access check tags have been _access_*, not *_access.
Does language_load() not have an injectable alternative yet?
As above.
t() should be an injected service.
Controller methods shouldn't be called "page callbacks", since that's not what they are anymore.
Just move code from that file back to the .module file. Don't bother with loadInclude() anymore. It's a ten year old hack. :-)
This should be called $entityManager. "$manager" tells me nothing about what it actually is.
Comment #38
wamilton CreditAttribution: wamilton commentedThis patch has Crell's suggestions implemented except for 2 because I couldn't find any evidence of such a thing...
git log -p -Slanguage_load
didn't show any case where it was removed for something else.Some of those tests that had numerous failures have less now.
Comment #40
wamilton CreditAttribution: wamilton commentedderp
Comment #42
disasm CreditAttribution: disasm commentedRenaming to prevent confusion.
Comment #43
piyuesh23 CreditAttribution: piyuesh23 commentedLooking into this.
Comment #44
Letharion CreditAttribution: Letharion commentedStraightforward re-roll.
Comment #45
disasm CreditAttribution: disasm commentedComment #46
disasm CreditAttribution: disasm commentedoops
Comment #48
Letharion CreditAttribution: Letharion commentedThis one isn't likely to work any better than the previous patch. It appears /user can't be loaded, which causes some tests to fail. Can't make any sense of it, but I'm re-rolling the patch onto the ContainerInjectionInterface change.
Comment #50
disasm CreditAttribution: disasm commentedThe last patch was still using ControllerInterface on the controller. Rerolling using ControllerBase instead. This should put us back at the same state as #44.
Comment #52
disasm CreditAttribution: disasm commentedThis is a massive overhaul:
Comment #53
plachI did not look at the patch in details, but I don't think this is correct: CT should be coded as it were a contrib module, the entity system should not depend explictly on it nor know of interfaces it provides. If we have a use case to make
getController()
public, then other modules exposing their own entity controllers might need that. I'd suggest to revert this change.Comment #54
plachComment #55
disasm CreditAttribution: disasm commentedrestored the public, and removed the custom method, although this controller can't use the public method, because it's constructor takes two arguments, so getting the class and creating a controller manually. Also, noticed that create and update are not completely identical. They have some different variable names, so fixed the access check. Local tests were passing with a couple exceptions that make no sense to me (illegal string offset menu_base_path, which I checked and is defined in the entity, so I'm not sure if it's a fluke or not.
Comment #57
xjmThanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.
Comment #58
Gábor HojtsyIn short, we have 3 more days to get this to RTBC or we'll need to split this to two issues and do it in 2 steps. Anybody wanna jump on this? :)
Comment #59
Gábor HojtsyComment #60
andypost// @todo revisit after https://drupal.org/node/2048223
$account = \Drupal::curentUser();
$this->prepareUser($account)
need empty line before end of class
Comment #61
disasm CreditAttribution: disasm commentedFixes most test failures. There's still an issue with translator not having permission.
Comment #63
disasm CreditAttribution: disasm commentedFinally!!! The operation was wrong in access check (should be update, not edit). Also, the edit() method of controller needed a type hint for Language to properly upcast. The test that failed above passed green for me locally.
Comment #64
jibranLet's convert this to OO or revert this.
Comment #65
disasm CreditAttribution: disasm commentedconverted!
Comment #66
jibranjust minor
We have
$this->entity
and$this->language
I think we don't need these.no need for this line.
Comment #67
disasm CreditAttribution: disasm commentedStoring the entity and language on the object is necessary for the questions and cancel route to be able to access them. Because the submit is a fresh request, it doesn't have access to those variables, so we pass them via form_state. Patch addresses some blank line issues.
Comment #68
disasm CreditAttribution: disasm commentedRemoving manual testing tag. This got plenty of manual testing in debugging test failures.
Comment #69
googletorp CreditAttribution: googletorp commented#67: drupal8.content-translation.module.1987882-67.patch queued for re-testing.
Comment #71
googletorp CreditAttribution: googletorp commentedDue to #2089635: Convert non-test non-form page callbacks to routes and controllers going into core some of this work has already been done, which also caused to patch to not apply anymore.
I have tried to resolve the merge conflicts I got putting the patch on core as it is right now.
I also removed the change to the EntityManager class which made the getController method public instead of private. This change might make sense, but I didn't see how it had any reference to this issue, so I removed that part of the patch.
Comment #73
googletorp CreditAttribution: googletorp commentedFixed the issue that caused the tests to fail.
Comment #75
googletorp CreditAttribution: googletorp commentedFixed the issue with the test cases - accessing unpublished translations and fixed a notice I came upon.
Comment #76
pfrenssen@return instead of @returns. Also don't forget to specify the type of data that is returned.
Spelling: entitiy.
Why change CRUD to CURD?
Don't remove this empty line. The curly brace that closes a class should be preceded by a blank line to distinguish it from one that closes a method.
Keep these in alphabetical order, with Drupal namespaces at the top, and third parties at the bottom.
This looks strange. Use empty($new_translation) or !$new_translation.
Keep the Core classes together, and in alphabetical order.
Has to be 'translated' instead of 'translationd' :)
It seems dirty to fall back to the procedural functions here. You should be able ti get the language_manager service from the dependency injection container and call isMultilingual() on it.
Alphabetical ordering.
Comment #77
disasm CreditAttribution: disasm commentedstraight reroll. Some of the comments above are addressed by the access checks and route already being in core with quick-and-dirty conversions.
Comment #79
disasm CreditAttribution: disasm commentedlooks like something changed since I pulled last night. Here's another.
Comment #81
disasm CreditAttribution: disasm commentedWorking off of #75, removing the access/route changes, the ones done in the quick convert are better than the ones existing in this old patch.
Comment #82
disasm CreditAttribution: disasm commentedWorking off of #75, removing the access/route changes, the ones done in the quick convert are better than the ones existing in this old patch.
Comment #84
disasm CreditAttribution: disasm commentedremoved some language_load calls and fixes hook_menu entry so Edit link shows up correctly so tests pass. Also, general cleanup including removing some temporary files accidentally committed in merge. As for comment above regarding use order, unless something has changed, to my knowledge there isn't a consensus as to order being alphabetical or not. My preference is towards using the order it shows up in the code, but I'm flexible if a decision is made one way or the other. I find having the thing it extends/implements, followed by the params to the constructor make the most sense to me, with all the Symfony stuff below the Drupal stuff.
Comment #86
disasm CreditAttribution: disasm commentedfixes route subscriber to use delete form and removes the wrapper controller.
Comment #88
vijaycs85Currently blocking #2102445: Remove drupal_set_title in content_translation module controllers
Comment #89
disasm CreditAttribution: disasm commentedfixes test failures.
Comment #90
tim.plunkettBeware: I made a misstep in #2091691: Convert test non-form page callbacks to routes and controllers with some of my changes to content_translation that is being addressed in #2102777-11: Allow theme_links to use routes as well as href. See the interdiff "2102777-11-tim.txt"
Comment #91
pfrenssenComment #92
pfrenssenThis needs some work after #2106349: Comment translation overview has broken local tasks got in, which now retrieves the entity from the request in ContentTranslationController, instead of having it passed in as an argument.
Comment #93
pfrenssenThere are some test failures on deleting translations, but I am unable to debug it since I keep hitting #2129039: Integrity constraint violation when translating body field and can't even get to a point where I have a valid translation to delete.
Comment #94
tim.plunkettThe LanguageConverter part seems overly complex. Upcasters are extremely expensive to run, if it can be avoided let's please do it.
Also, #2086479: Convert content_translation_delete_confirm() to the new form interface was marked as a dupe, but that contains one of the last 2 confirm_forms(). I might steal that one back...
Comment #95
tim.plunkettOkay because I want to kill confirm_form ASAP, I reopened #2086479: Convert content_translation_delete_confirm() to the new form interface.
Comment #96
Gábor HojtsySo #2086479: Convert content_translation_delete_confirm() to the new form interface was swiftly committed. Those parts would need to be removed from this patch. As for the upcasting, 'language_entity' upcasting would be done by the existing upcasting infrastructure without further work. That does not upcast to an object that we use for type hinting elsewhere though :/ All the fault of CMI's circular dependency on language stuff and storing languages as config entities.
Comment #97
vijaycs85Re-roll + Removing delete_confirm() part
Comment #99
vijaycs85Reroll...
Comment #101
kim.pepperComment #102
vijaycs85Fixing the URL issue...
Comment #105
vijaycs85Still test failing because of $entity missing in edit. But has further improvements from #102
Comment #107
vijaycs85Fixing test fails...
Comment #110
vijaycs85Minor fix on edit() method for the test fail. Not bug, but an observation is we don't really use content translation edit path (e.g. /node/1/translations/edit/fr) and use the language prefixed path of edit (e.g. /fr/node/1/edit).
Comment #111
kim.pepper110: 1987882-content-translation-110.patch queued for re-testing.
Comment #113
kim.pepperThis is a straight re-roll of #110.
Comment #116
neetu morwani CreditAttribution: neetu morwani commentedLAst patch rerolled. Now the patch applies successfully.
Comment #118
Sill CreditAttribution: Sill commentedComment #119
Alumei CreditAttribution: Alumei commentedPatch from #116 still applies successfully.
Comment #120
victor-shelepen CreditAttribution: victor-shelepen commentedRerolled.
Comment #121
dawehnerThere ain't no hook_menu for that, afaik we dropped all that code anyway./
Comment #123
andypostThis should not be removed!!!
Comment #124
victor-shelepen CreditAttribution: victor-shelepen commentedI've rerolled the path 110, have modified it. It works while the manual testing. I see some tests fails. I hope somebody will make useful comments.
Comment #126
victor-shelepen CreditAttribution: victor-shelepen commentedManual testing works well. Some tests fail. I guess somebody give an advise.
Comment #128
victor-shelepen CreditAttribution: victor-shelepen commented126: content_translation-add_page-1987882-126.patch queued for re-testing.
Comment #130
victor-shelepen CreditAttribution: victor-shelepen commentedIt seems work. It is not finished yet. I need get some comments about.
Comment #132
victor-shelepen CreditAttribution: victor-shelepen commentedComment #133
victor-shelepen CreditAttribution: victor-shelepen commentedComment #135
victor-shelepen CreditAttribution: victor-shelepen commentedIt duplicates this one https://drupal.org/node/2224607. We need an authorized person who is able to take a right decision.
Comment #136
victor-shelepen CreditAttribution: victor-shelepen commentedComment #137
andypostClosed #2224607: Move all functions from content_translation.pages.inc file to Drupal\content_translation\Controller\ContentTranslationController class as duplicate of this
Looks mostly RTBC just minor nitpicks:
trailing whitespace
please add new line
any reason for?
Comment #138
victor-shelepen CreditAttribution: victor-shelepen commented3. <- I fixed tests because they are failed. I looked what tests had generated, and I modified tests. It works.
1. <- I can not understand. Sorry.
Comment #139
victor-shelepen CreditAttribution: victor-shelepen commentedComment #141
victor-shelepen CreditAttribution: victor-shelepen commentedComment #142
andypostJust minor nitpicks
trailing whitespace
needs new line
Comment #143
YesCT CreditAttribution: YesCT commentedfixing whitespace nits mentioned in #137 and #142.
@likin attaching screenshots to maybe help explain why this bothered andypost. you should be able to do a git diff and look to see unintended changes like this, also looking at your patch in dreditor after you upload it to an issue might be useful. https://dreditor.org/
I have other comments I'll make separatedly.
Comment #144
YesCT CreditAttribution: YesCT commentedjust pointing out and fixing some nits.
another very small nit, I'll just fix.
too many newlines at end of file,
and at end of class, should be a newline before the closing class curly brace.
https://drupal.org/node/608152#indenting
"Leave an empty line between end of method and end of class definition:"
similar moving of an @todo (we can create the issue todo that, and add the link). (or do a follow-up to do all those @todo's moved in this issue, but not created by this issue)
comment should wrap at 80 chars.
"Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over,"
https://drupal.org/coding-standards/docs#drupal
since all these use's are new, might as well add them in alphabetical order.
wow. this seems pretty huge. I wonder if there was another issue for this @todo.
This @todo came from #2089635: Convert non-test non-form page callbacks to routes and controllers.
Nothing wrong here, just will link to that related issue.
This is probably an english confusion.
Standards say:
"summary lines must start with a third person singular present tense verb, and they must explain what the function does. Example: "Calculates the maximum weight for a list.""
https://drupal.org/node/1354#functions
This first word does end in an 's" but it's not a verb saying what it does.
fixing this to say "Builds ..."
this @todo either needs to be taken care of in this issue, or a separate issue should be opened for it, and a link added in the comment for that issue number.
sigh. this code already existed, and is just being moved as part of this conversion. but still.. would be good to see if an issue already exists for this, and if not, make one.
similar third person verb change here.
got distracted actually reading the patch.
trying to get back to just nits, and then reread it later.
here, \Drupal
https://drupal.org/node/1354#file
posting just the nits, following is coming a review (just reading the patch)
Comment #145
YesCT CreditAttribution: YesCT commenteda.
I was wondering about the re-ordering of parameters, and when I looked at other places in core, it does seem like when building page, we do put the request as the last param. ok.
b.
but, I do not see a default value for the $language, so this isn't optional...
there are a lot of places like this. not fixing it here.
Should do a separate patch/interdiff for those.
c.
I'm also super unsure about what the correct thing to do here is. Should these be "forms"?
I dont see many other add edit methods on controllers in core, or as children of the convert meta this belongs to.
this todo is a new one in this issue, is it intended to be fixed before this patch is committed?
is it related to this other @todo?
hm. wonder if #2139135: Unit test \Drupal\config_translation\Routing\RouteSubscriber is relates or will be effected.
huh?!
we stop and do not do the rest of this test?
why do we have to change this test? why do we have to provide data now?
I dont understand why we have to change this part of the test either.
ok. this is fixing an already existing copy and paste problem.
really, maybe this should be a novice follow-up as it is not related to doing this conversion.
this implies we are not editing (saving something already published) but are creating... why changing from creating to editing?
Comment #146
YesCT CreditAttribution: YesCT commented1. I was looking closely at just edit(). (should do the same for add())
1a.
1b.
2. wrong namespace for the Language class for the $language param.
3. Also, the method content_translation_edit_page from the .inc file had default values for the params. I dont know why we are changing that here.
--------------
4.
I started to do this:
4a. But I'm *really* not sure about the change to not have default values.
4b. Why did we make them required?
4c. Is that change what rippled through other changes here (including tests and ContentTranslationRouteSubscriper::alterRoutes() ?)
4d. And if they really are, then we should update the docs taking out (optional).
Comment #147
dawehnerI do like using Request as last, as the passed in Language is more important. On top of that this makes it more consistent with the add method for example.
As talked in IRC, the default values are moved onto the routing object.
They are indeed something like entity forms, but they aren't really, as they wrap existing entity forms and aren't new ones. I consider the current code as okay for now.
The problem existed before. Nothing ensures that the 'default' operation is "default", but afaik all core entity types provide some form controller type called 'default'.
Improved the comment by better describe what is actually going on.
Afaik not really. THis is the content translation module, not config.
This probably improves the performance of the tests but is not intended!
On top of that I removed the changes in the tests, because we really don't have to, ... additional improvements can always be done later.
Comment #149
dawehner@yesct++
Comment #150
dawehnerForgot the interdiff.
Comment #151
andypostSuppose now it's ready
Comment #153
vijaycs85149: content_translation-1987882-149.patch queued for re-testing.
Comment #155
vijaycs85Comment #156
penyaskitoAwesome work!
Do we need an issue here?
Shouldn't we use LanguageInterface when possible?
Comment #157
vijaycs85thanks @penyaskito. here is the update:
#156.1 - Added #2250841: Adding a inline template for content translation status
#156.2 - Fixed them all in ContentTranslationController.
Comment #159
vijaycs85157: 1987882-content_transaltion_controller-157.patch queued for re-testing.
Comment #160
tim.plunkettComment #161
penyaskitoEveryone concerns have been already addressed.
Comment #162
alexpottNo longer used and language_negotiation_get_switch_links() does not exist either (since #1862202: Objectify the language system). This patch removes all the usages so it should remove the function. Hah... I see this patch is not copying the code correctly - this is so stale... In HEAD this is calling
\Drupal::languageManager()->getLanguageSwitchLinks
. Anyhow we can just remove.This function is no longer used as this patch removes all the usages. Let's get rid.
Hmmm? I'm guessing the first $url assignment can be removed. But then again the comment says link the fields tab? What is desired here?
Not used.
Comment #163
victor-shelepen CreditAttribution: victor-shelepen commented#162
Comment #164
victor-shelepen CreditAttribution: victor-shelepen commentedComment #166
victor-shelepen CreditAttribution: victor-shelepen commented157: 1987882-content_transaltion_controller-157.patch queued for re-testing.
Comment #168
victor-shelepen CreditAttribution: victor-shelepen commentedI've done small changes according to two commits from #2240463: Fix text: languages are not "enabled/disabled" anymore, they are added/removed and #2225739: Remove usage of field_info_instances(), use EntityManager::getFieldDefinitions() instead.
Comment #169
dawehnerThis change seems to be a step back.
I wonder whether we can already use the some route here?
Those ones could be $this->t()-ified
Comment #170
victor-shelepen CreditAttribution: victor-shelepen commentedThe comment #169 has been processed.
1, 3 - resolved.
2 - It invokes some logic problems. Entity has methods: url and getSystemRoute. But They return a string not a route object. Yes we can generate an Url object from url-string and than to a renderable array. It is weird.
Comment #172
kim.pepperDid a re-roll for PSR4. Also injected some more dependencies and updated for some recent Entity API changes. Code style fixes too.
Comment #174
kim.pepperForgot to add @language_manager to the service definition.
Comment #175
Crell CreditAttribution: Crell commentedEr, shouldn't this just be gotten from the method parameters?
We're trying to eliminate access to request attributes from controllers et al. What's _entity_type_id? If it's an attribute, you can just get it in the method signature by having a parameter of the same name.
Which suggests it should be entity_type_id, no _.
The controller method is way too long and involved, but that's just a straight port of the old callback function so we shouldn't block on that.
Comment #176
katbailey CreditAttribution: katbailey commentedThis addresses Crell's first point. I have no idea how to address the second :(
I did notice that the delete link was using the edit route so I fixed that. Oh and renamed a variable.
Comment #178
kim.pepperComment #179
kim.pepperFixes for #175
Comment #181
kim.pepperDiscussed with tim.plunkett and current convention is to use params with underscores (e.g. $_entity_type_id) rather than pulling it out of $request->attributes.
This reverts my previous changes of using $entity_type instead of $entity_type_id
Comment #183
kim.pepperI removed the call to the procedural
content_translation_controller()
and instead call$this->entityManager->getController($entity->getEntityTypeId(), 'translation');
Also, fixed an issue where strings are being passed in, but the code still expects a language object.
Comment #185
kim.pepperThis fixes a number of issues, including loading languages correctly, ensuring that translation routes are admin paths, and using ->getId() instead of the property directly.
Also, the logic in
ContentTranslationManageAccessCheck
is pretty complex, so adding a unit test in here.Comment #186
Gábor Hojtsy@kim: let's keep focusing on the scope of this issue. If you make changes unrelated it makes it harder to review and adds more to debate about, that is farther from the goal. See #2276387: Translate routes should properly inherit admin path use from edit route for an RTBC issue that makes the translation paths admin or not admin based on proper admin status inheritance from the edit route. That is the correct solution to that problem. Eg. comments don't have any admin pages, if you edit them, delete them, etc. are all frontend, so the translation should be as well. Please remove these unrelated changes. Let's focus on making the controller conversion happen. This has been lingering for so so long.
Comment #187
kim.pepper@Gabor, sorry. I thought it was a mistake.
This also puts back in a fix the @katbailey made in #176 that somehow I left out in my re-roll.
Comment #189
kim.pepperRe-roll after #2246665: Typehint with Drupal\Core\Language\LanguageInterface instead Drupal\Core\Language\Language went in.
Also, fixes "Route "content_translation.translation_delete_node" does not exist." test fails.
Comment #190
YesCT CreditAttribution: YesCT commenteddoes this refer to #1537452: Generalize hook_node_prepare() with hook_entity_prepare()
or maybe referring to hook_entity_prepare_form() now
or
EntityForm::prepareEntity()
?
I asked in #145 and @dawehner answered in #147:
but the comment is still there.
Is there an issue we can link to in the todo to ensure there is a default operation?
Do we need to create the issue,
or just take out the @todo, and continue to assume it's always 'default'?
looks like this is the same todo. So maybe I should just open the issue and replace these two @todos.
Is this related to #2006348: Remove default/fallback entity form operation ?
Well, made #2284043: Provide a way to figure out the default form operation with a default_operation annotation on the entity.
===============
small clean ups and removing out of scope changes.
Comment #191
kim.pepperI noticed the Language param converter isn't converting the language params for the access checker. Not sure if it's meant to? or is it just for the routes?
Comment #192
kim.pepperSorry, I don't really have answers to the questions in #190. Unassigning.
Comment #193
plach@YesCT:
1: That should refer to #1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language.
2: That should refer to #2006348: Remove default/fallback entity form operation.
3: Yes, those comment should match and refer to #2006348: Remove default/fallback entity form operation.
Why we have an underscore prefix? Is this a WSCCI convention?
In other parts of this patch we are using just $source and $target. Please let's be consistent and pick one of the two forms.
Those parentheses wrapping the variable are unnecessary, are they supposed to improve readability?
We are inside a routing controller, please let's call the variable $handler to avoid confusion :)
For consistency let's use a multiline array here too.
We can simplify this code by just returning
!empty($definition['type']) && $definition['type'] == 'language'
Comment #194
kim.pepperComment #195
penyaskitoReroll of #190.
I'm not sure if we should use 'entity' or $entity_type_id, but tests will tell.
Comment #197
penyaskitoIt should be $entity_type_id. One test namespace was wrong too, this one should be green.
Comment #199
penyaskitoAnd now after changing the namespace the use statement is missing.
Comment #200
penyaskito#193.2: $source and $target are already used on this scope. Do we really want to override them?
I don't have answers for #191, sorry.
The attached patch adds the issue urls for todos, and fixes #193.3, 4 and 6. The other remarks in #193 were already answered or not relevant anymore.
Comment #201
tim.plunkettThis
implements ContainerInjectionInterface
is redundantYou don't need to inject either of these, it's provided by ControllerBase.
When is this not true?
Please put these on one line.
No space between the & and $
We don't use getInfo() anymore, please use an annotation on the class.
Comment #202
penyaskitoThanks for reviewing Tim.
Attended every comment from #201. For #201.3, I think it is never true in core (see ContentTranslationRouteSubscriber) so I removed the if clause.
Comment #203
tim.plunkettI think this is good to go. There's still some ugly code in here, but that's just being moved around. The new bits all look good.
Comment #204
alexpottNot used
Couldn't we type hint here on ContentEntityInterface?
use ->getid() instead of ->id
Comment #205
kim.pepperThese look like quick fixes. I can take a look.
Comment #206
kim.pepperFixes for #204.
Comment #207
penyaskitoRerolled #206, automatic merge.
Comment #208
andypostComment #210
penyaskitoA route name was wrong, not sure what changed or why didn't fail before.
Comment #211
andypostso back to rtbc
Comment #212
Crell CreditAttribution: Crell commentedLet's go ahead and remove the page callback function we're replacing, too. Should be an easy reroll.
Comment #213
kim.pepperThis removes content_translation.pages.inc which contains the callback functions as per #212 as well as helper functions that were used by those callbacks.
Comment #214
andypostGreat clean-up! Just minor nitpicks, form state uses methods consistently in core
any reason for underscore prefix? $entity_type_id all over core
let's name it properly - $handler
please, use methods
Comment #215
penyaskitoFixed #214.1, 214.2.
About 214.3, it is not a FormStateInterface object but $form_state_addition array. Renamed the variable for clarifying.
This is confusing, should we create another issue for changing that parameter to a proper FormStateInterface?
Comment #217
penyaskitoThis should work, the routing is dynamically generated and forgot to look at the subscriber.
Comment #219
penyaskitoArgh, forgot one.
Comment #220
penyaskitoComment #221
kim.pepperThis is a symfony convention. For route callbacks, any special request attribute that is not a parameter has an underscore prefix.
Comment #222
penyaskito@kim.pepper: You are right about Symfony convention. I did a quick research on core code and we are not really enforcing this convention.
Most of the request attributes don't use underscored names. I would say that we move forward on this issue, and if we want to discuss a policy about this let's open a new issue. There aren't too much cases looking at $request->attributes->get() calls.
EDIT: s/All/Most of.
Comment #223
Crell CreditAttribution: Crell commentedIt's a little more subtle than that. _-prefixed attributes are reserved for things that have special magical meaning to the system. Unprefixed values that are "meant" to be passed to the controller as arguments are fine.
Comment #224
andypostNice clean-up, first!
Now the code uncover another wtf - how you get entity from
entity_type_id
and what it's actually is?is this an entity ID or entity type ID?
Also order of name and type in @var is deffirent #2305593: [policy] Set a standard for @var inline variable type declarations
the logic here is different...
the same
Comment #225
penyaskitoFixed the @var order.
The difference between the access code and the controller is because access subscriber runs with a higher priority than convert params subscriber, so the conversion is not done when access code runs.
This should be ready to RTBC.
Comment #226
andypostSuppose it's ready
Comment #227
alexpottCommitted 77c666c and pushed to 8.0.x. Thanks!
Fixed during commit.
Comment #229
Gábor HojtsyThis caused a major bug at least: #2325977: All links lead to same entity translations in translation overviews.