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.
Remaining tasks:
- (novice) create follow-ups (See #90).
Comment | File | Size | Author |
---|---|---|---|
#86 | 1987854-diff-79-86.txt | 1.57 KB | vijaycs85 |
#86 | 1987854-system_themes_page-86.patch | 21.24 KB | vijaycs85 |
#78 | 1987854-system-theme-page-78.patch | 21.12 KB | kim.pepper |
#77 | interdiff.txt | 3.04 KB | kim.pepper |
#77 | 1987854-system-theme-page-77.patch | 20.98 KB | kim.pepper |
Comments
Comment #1
sidharthapComment #3
dtarc CreditAttribution: dtarc commentedI'm going to take a look at this today at drupalcon portland.
Comment #4
andypostRelated #2010086: Replace theme() with drupal_render() in system module for theme_system_themes_page()
Comment #5
E.Kurko CreditAttribution: E.Kurko commentedI'm going to check it today on CodeSprintUA
Comment #6
E.Kurko CreditAttribution: E.Kurko commented#1: system-theme-page-1987854-1.patch queued for re-testing.
Comment #8
sidharthapSubmitting a fresh patch. But still getting a warning message in local dev.
Invalid argument supplied for foreach() in theme_system_themes_page() (line 1497 of core/modules/system/system.admin.inc).
Comment #9
sidharthapChanging status.
Comment #11
tim.plunkettThis blocks removal of MENU_LOCAL_ACTION.
Comment #12
dawehnerLet's inject the config factory.
Just wondering whether we have a Crypt component in core which has a getToken method and uses everything we need here? (out of scope of this issue for sure).
Comment #13
tim.plunkettWe have \Drupal\Component\Utility\Crypt, but it only has randomBytes(), hmacBase64(), hashBase64(), and randomStringHashed().
Comment #14
tim.plunkett.
Comment #15
msmithcti CreditAttribution: msmithcti commentedThis is an initial straight conversion that should hopefully get the tests passing. The controller method will need some more cleanup and looking at the tags the local actions converting.
Comment #17
msmithcti CreditAttribution: msmithcti commentedLooks like I spoke too soon! This patch cleans up the documentation, injects the translation service and sets the route name in hook_menu().
Comment #18
dawehnerIs there any reason to not return a render array instead?
there is now the csrf_token service available to be used.
We could think whether this new form should directly be a form controller.
config() should be replaced with Drupal::config()
It should be _content instead of _controller
Comment #19
msmithcti CreditAttribution: msmithcti commentedJust gave converting this to a render array a go. For some reason the theme definition in system_theme() is missing an entry for the theme_group_titles variable, meaning it doesn't get passed through to the theme function. Looking at the latest patch from #2009674: Replace theme() with drupal_render() in system module this issue it being fixed there. The other issues in #14 should be addressed.
Comment #20
dawehnerIt should be ModuleHandlerInterface in these three places so you can swap that theoretically.
Let's return a render array instead of render the string.
Comment #21
pfrenssenHere is the parentheses police! These were all present in the original code, but let's clean them up now that we have the chance :)
It is not needed to wrap the comparison in the middle in parentheses. Comparison operators such as != have precedence over || and &&.
There's also no need to wrap the final !isset() in parentheses. That is just a single function call.
Doesn't need to be wrapped in parentheses. && has precedence over the assignment operator.
Same as above, parentheses are not necessary. See PHP operator precedence.
Comment #22
pfrenssenDid not intend to remove those. I guess we don't need CodeSprintUA any more?
Comment #23
tim.plunkettParentheses also increase readability. It's not just about precedence.
Comment #24
pfrenssenI don't know, for the first one I might agree, but the last two are less readable to me. These look very odd to my eyes, making me pause to inspect them closely to see what kind of trickery is going on in there.
Comment #25
msmithcti CreditAttribution: msmithcti commented@dawehner on returning a render array @see comment #19 - it feels out of scope to me for this issue to fix system_theme(), but happy to take your lead on that. I'm now using ModuleHandlerInterface but I also along those same lines modified the injected translator to use TranslatorInterface instead of TranslationManager - I'm assuming that was the right move?
@pfrenssen I've removed some of the parentheses but I agree with Tim that in most cases they increase readability.
Comment #26
tim.plunkettActually, it should stay as $this->translation, #2049709: TranslationManager::translate() should be on an interface will be in soon.
Comment #27
msmithcti CreditAttribution: msmithcti commentedArgh, just when I thought I was starting to get when to type hint with the interface vs. the class. I'm including an interdiff between #19 and #27 which excludes the cruft from changing the TranslationManager type hint.
Comment #28
tim.plunkett#2049709: TranslationManager::translate() should be on an interface went in!
Comment #29
msmithcti CreditAttribution: msmithcti commentedGreat, this patch replaces the use of TranslationManager with TranslationInterface.
Comment #30
dawehnerSecond point of #1987854-20: Convert system_themes_page() to a new style controller is still true, here is a simple fix.
Comment #31
andypostCould be merged, $admin_form useless
Any reason to move the code around?
Sort helpers needs more clean-up:
system_sort_themes()
could be a method of the controllersystem_sort_modules_by_info_name()
used in controllers that needs to removemodule_load_include()
Comment #32
msmithcti CreditAttribution: msmithcti commentedInitially no (I was just following on from the previous patch), however after looking at system.admin.inc it mainly contains theme functions, controllers and form controllers so will hopefully not be around much longer.
All points should be covered in this patch.
Comment #33
disasm CreditAttribution: disasm commentedRerolling for ControllerBase and ContainerInjectionInterface. Getting rid of moduleHandler and translation injections, switching to $this->t() and $this->moduleHandler(). Using configFactory instead of a specific config object because that's what most of core is doing at this point, and not that this class will be extended, but it gets really confusing when you extend a class trying to figure out what $config is the right one.
Comment #34
jibranSorting order is not correct.
Comment #35
msmithcti CreditAttribution: msmithcti commented@jibran - what is the correct sort order for use statements? I had a quick glance at the coding standards but couldn't see anything there.
Comment #36
jibranWe don't have a coding standard for this but alphabetic ascending order with vendor namespaces at the end will work for now.
Comment #37
disasm CreditAttribution: disasm commentedI disagree with alphabetical. My preference would be classes the controller extends/implements first, anything injected next, rest of Drupal related stuff, all of vendor stuff for the order. That way it matches the order things appear for the most part in the class itself. Also, when something is added or removed from create, it's easy to find. That being said, since there isn't a standard, lets not hold up the issue because of the sort order. We can always open an issue to add a recommended sort order to the coding standards, just not here.
Comment #38
dawehnerDoes the controllerBase provide you $this->config($name) so you don't have to manually inject that?
Comment #39
disasm CreditAttribution: disasm commentedIt does. Also replacing drupal_set_title with _title on route.
Comment #40
dawehnerOh I am so sorry, here is a trailing whitespace.
This comment is invalid/not needed anymore.
Do you also plan to convert drupal_valid_token to the csrf generator service?
Comment #41
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 #42
disasm CreditAttribution: disasm commentedAddresses comments in #40.
Comment #43
dawehnerI think we should just use normal injection ... anyway there are multiple codestle issues on here.
Comment #44
disasm CreditAttribution: disasm commentedGoes back to #33. Last few patches were from the wrong issue.
Comment #46
disasm CreditAttribution: disasm commentedComment #47
disasm CreditAttribution: disasm commentedForgot a comment... This resolves issue committed here: #2067017: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants
Comment #48
jibranThis is not recommended anymore.
Comment #49
disasm CreditAttribution: disasm commentedThat function doesn't exist in core anymore... Removing.
Comment #50
jibranSee drupal_get_form we can convert
system_themes_admin_form
to OO and pass a class name.Let's convert this form to OO.
Comment #51
tim.plunkettYou should wait on these conversions until Phase 1 of the new conversions is done, see #1971384-43: [META] Convert page callbacks to controllers
Comment #52
msmithcti CreditAttribution: msmithcti commentedHere is a plain old reroll and doesn't include any items from the reviews above.
Comment #54
msmithcti CreditAttribution: msmithcti commentedOops - missed injecting the token generator service.
Comment #56
msmithcti CreditAttribution: msmithcti commentedArgh ... another reroll!
Comment #57
msmithcti CreditAttribution: msmithcti commentedConverted system_themes_admin_form to FormBase, and replaced the deprecated call to drupal_theme_access with ThemeAccessCheck.
Comment #58
tim.plunkettYou can use $this->t()
Comment #59
msmithcti CreditAttribution: msmithcti commentedNow using the injected translationManager instead of t().
Comment #60
pfrenssenKeep these in alphabetical order.
It is "An HTML string" instead of "A HTML string". This line should also be indented with one more space.
Wouldn't it be nicer to use an anonymous function for this?
Comment #61
disasm CreditAttribution: disasm commentedAddresses 2 and 3. As for #1, alphabetical is not in the coding standards. If someone wants to open an issue to add it to the coding standards, feel free, but until it's discussed I see no reason to enforce it.
Comment #62
pfrenssenAbout the alphabetical ordering, it's not yet in the coding standards, but I saw this mentioned in several issues already. There already is an issue about it as well: #1323082: Establish standards for namespace usage [no patch]. The latest version of the proposed guidelines in that issue mention:
So it looks like it will be part of the coding standards in the future, even though it might take a while, that issue has not moved in months. Anyway, you're absolutely right that it is not currently part of the coding standards so it does not need to be addressed for this issue to pass. I just find it a bit sad to mess up a perfectly ordered list.
The anonymous function is indented 2 spaces too deep.
This is replaced by the anonymous function, so it is not needed any more.
This code has moved to another position in the file, but does not seem to have changed in any way, is this intentional?
Comment #63
heddnComment #64
vijaycs85Updating changes for review comments in #62
#62.1 - Fixed
#62.2 - Fixed
#62.3 - Not fixed - it is moved from system.admin.inc to system.module to avoid inc include just for this function.
Comment #65
dawehnerJust two small points:
Enabling, disabling, and setting an admin theme still works fine.
Short ads block: #2111349: Move format_plural to the string translation service and format_interval to the date service.
drupal_get_form can now we replaced by the form_builder service.
New line between them needed.
Comment #66
vijaycs85Thanks for the review @dawehner. here is the update on them:
#65.1 - NOT FIXED - has to wait until[#2111349] lands
#65.2 - FIXED
#65.3 - FIXED
Comment #67
tim.plunkettYou don't need to `use` this or call `new` yourself. Just
$this->formBuilder->getForm('Drupal\system\Form\ThemeAdminForm', $admin_theme_options);
And yes, that means you should inject the form builder. Or add it to ControllerBase.
Comment #68
msmithcti CreditAttribution: msmithcti commentedThis should fix the issues raised in #67.
Comment #70
vijaycs85updating changes for #67.
Comment #71
tim.plunkett70: 1987854-system_themes_page-70.patch queued for re-testing.
Comment #73
kim.pepperComment #74
kim.pepperComment #75
kim.pepperRe-roll.
Comment #77
kim.pepperFixes issue with CSRF token by using route_names instead of urls. Two birds with one stone?!
Comment #78
kim.pepperThis cleans up the themesPage() a little bit more:
Comment #79
kim.pepperAdded missing comment on $themeHandler property.
Comment #80
dawehnerIt is great to see it in use!
Comment #81
xjm79: 1987854-system-theme-page-79.patch queued for re-testing.
Comment #82
kim.pepper"Re-test request ignored because test is already queued."
Comment #83
webchickThis all looks good, except this part...
I don't understand why this was done. We introduced an API change and also removed the documentation (such that it was) that explains what this convoluted code is doing.
I think we should do what we did to system_sort_modules_by_info_name() here and simply move the function to system.module for now. Then probably a follow-up to discuss the fate of these functions. (For example, they probably make more sense off the ModuleHandler class or whatever the equivalent of that is for themes.) It's out of scope for this issue though, IMO.
Comment #84
neclimdulWe could just document the anonymous function better and actually explain what its doing. That's some weird logic and it should have that anyways instead of just lying about sorting by name.
Comment #85
webchickWe could, but it's not part of converting this from a page controller to a controller controller, so let's discuss it in a separate issue.
Comment #86
vijaycs85Fixing #83....
Comment #87
kim.pepperThis resolves the issues in #83 so back to RTBC
Comment #88
webchickGreat, thanks!
Committed and pushed to 8.x.
Comment #90
YesCT CreditAttribution: YesCT commentedI didn't read all the comments on this issue, but I did see at least in #83 and #84 one or two (I'm not sure) follow-ups are asked for.
tagging so we dont forget.
could be a novice issue to read through the comments and identify other issues that need to be created, and link them back to this issue (using the related meta data field on issues, and also just making a comment here that they were created. that will help the people who are familiar with this issue look at the new issues to make sure the summaries are accurate.)
when someone reads all the comments and verifies all follow-ups are created, the needs follow-up tag should be removed.
the dreditor clone button can be helpful for autofilling similar components and such. note that new follow-ups in this case should probably be status active.
Comment #91
YesCT CreditAttribution: YesCT commentedI'm not sure what "status" to use for this.
trying this with a note in the title that the issue is fixed.
Comment #92
andypost#83 is addressed in commited patch. So it's not clear what's left here
Comment #93
alexpottYep #86 reverted the contentious changes.