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.
Comment | File | Size | Author |
---|---|---|---|
#133 | interdiff.txt | 659 bytes | kim.pepper |
#133 | 1987848-theme-default-133.patch | 10.18 KB | kim.pepper |
#130 | interdiff.txt | 2.25 KB | kim.pepper |
#130 | 1987848-theme-default-130.patch | 10.18 KB | kim.pepper |
#128 | 1987848-system-theme-default-128.patch | 9.82 KB | akozma |
Comments
Comment #1
vijaycs85Initial patch...
Comment #2
dawehnerContains ...
It would be really helpful to describe the controller + method better.
Needs better docs.
Lets inject the config in order to set it.
You could return a RedirectiResponse instead.
No need for keeping the hook_menu entry.
Comment #3
dawehner.
Comment #4
dtarc CreditAttribution: dtarc commentedI'm going to take a look at this today at drupalcon portland.
Comment #5
InternetDevels CreditAttribution: InternetDevels commentedAs this hasn`t been fixed yet we are going to work on this issue today during Code Sprint UA
Comment #6
InternetDevels CreditAttribution: InternetDevels commentedPatch which fixes issues above attached.
Comment #7
dawehnerThere seems to be no real reason to document that bit.
I'm not sure whether we want to store the system.theme config object or the full config factory.
The config factory is a different class.
Nice!
OH i see now. ... just use $this->config then and get rid of the configFactory object.
Comment #8
InternetDevels CreditAttribution: InternetDevels commentedChanges added.
Comment #9
dawehnerCool stuff!!
Comment #10
alexpottNeeds a reroll
Comment #11
pwieck CreditAttribution: pwieck commentedRe-roll. Sorry don't have a handle on making interdiff.txt file yet.
Comment #12
dawehnerThe yml file failed, which looks nice now. Back to RTBC
Comment #13
alexpottNeeds a reroll...
Comment #14
pwieck CreditAttribution: pwieck commentedWorking on re-roll
Comment #15
pwieck CreditAttribution: pwieck commentedRe-rolled again.
Comment #17
pwieck CreditAttribution: pwieck commentedNeeds work, removed tag
Comment #18
pwieck CreditAttribution: pwieck commented#15: system-route-system-theme-default-1987848-15.patch queued for re-testing.
Comment #19
pwieck CreditAttribution: pwieck commented#15 passed and ready for review
Comment #20
dawehnerBack to RTBC.
Comment #21
alexpottYet another reroll...
Comment #22
rgoodine CreditAttribution: rgoodine commentedWorking on the reroll
Comment #23
rgoodine CreditAttribution: rgoodine commentedRerolled
Comment #25
pwieck CreditAttribution: pwieck commentedReroll #23 doesn't look right.
This is being removed in HEAD
So this shouldn't be added to patch
Comment #26
pwieck CreditAttribution: pwieck commentedDisregard #25. I don't know what I was even thinking. I just removed what was supposed to be there. :-(
Comment #27
pwieck CreditAttribution: pwieck commentedAs soon as #25 fails I have new file.
Comment #28
pwieck CreditAttribution: pwieck commentedre-re-re-roll. Minor conflict fixed.
Comment #29
star-szr#23: 1987848-23.patch queued for re-testing.
Comment #30
star-szr#23 is the patch to review, re-testing now. I worked with @rgoodine yesterday to reroll the patch.
Thanks for working on this @pwieck but I think the patch in #28 is just slightly off - the conflict was a bit tricky on this one.
Snippet from patched system.routing.yml in #28:
system_theme_settings_global is missing 'requirements'. Compare to patched system.routing.yml in #23 where both system_theme_settings_global and system_theme_default specify 'requirements':
Comment #31
ParisLiakos CreditAttribution: ParisLiakos commentedComment #32
ParisLiakos CreditAttribution: ParisLiakos commentedtag:)
Comment #33
star-szrJust want to note that normally we reroll the latest patch but in this case we should reroll from #23.
Comment #34
disasm CreditAttribution: disasm commentedreroll of #23.
Comment #35
ParisLiakos CreditAttribution: ParisLiakos commentedlooks good thanks!
Comment #37
ParisLiakos CreditAttribution: ParisLiakos commentedActually, lets inject url_generator as well
Comment #38
dawehnerJust a question for later: do we want to use the route name if existing later or always rely on the path?
Comment #39
ParisLiakos CreditAttribution: ParisLiakos commentedi guess using route name would be more solid, cause that would mean someone could change a route url and nothing breaks:)
Comment #40
dawehnerBut you can't replace the route by adding a new one, well these are things which have to be figured out later.
Comment #41
alexpottt is injectable... we have the string_translation service on the container..
Comment #42
h3rj4n CreditAttribution: h3rj4n commentedComment #43
h3rj4n CreditAttribution: h3rj4n commentedI removed the 'CodeSprintUA'-tag. I guess after more than a month that sprint is over ;)
I rerolled the patch and changed the translate function to a container. I took
\Drupal\system\Form\ModulesListForm
as example.Comment #44
h3rj4n CreditAttribution: h3rj4n commentedComment #45
dawehnerNothing sets the translationManager onto the object.
Comment #47
h3rj4n CreditAttribution: h3rj4n commentedThis one should work.
Comment #48
Crell CreditAttribution: Crell commentedDon't use $request->get(). Use $request->query->get(), so it's clear where it's coming from.
This is a Novice-able reroll.
Once that's in, though, we should refactor this code entirely. Having the same controller do the page display and the update is very wrong. Having configuration change on a GET request is even more wrong, even if there's a token on it.
At the very least we should split this up into two controllers, one that actually makes the change and then redirects back to the other display-only controller. But I'm OK with that being a follow-up. For now, let's just fix the above and get this in.
Comment #49
robeano CreditAttribution: robeano commentedI updated the patch to use $request->query->get() as requested. This could use a review.
Comment #50
Crell CreditAttribution: Crell commentedInterdiff please?
Comment #51
pfrenssenHere's the interdiff between patches from #47 and #49.
Comment #53
disasm CreditAttribution: disasm commentedreplace translationManager with t. Also, cleaned up boilerplate code in controller.
Comment #55
disasm CreditAttribution: disasm commentedComment #56
disasm CreditAttribution: disasm commentedComment #58
pfrenssenI had a look at the test, but it is green for me locally.
I added missing documentation for a parameter.
Comment #60
disasm CreditAttribution: disasm commentedI can't replicate these errors locally either. If anyone has any suggestions, I'm all ears.
Comment #61
star-szrThe tests are failing locally for me, seeing if I can fix things up.
Comment #62
star-szrWell this was fun :)
Main changes:
Comment #63
star-szrHere is the promised comment rewrapping, got left out accidentally.
Comment #64
star-szrRerolled for #2057589: [Docs clean-up] Rename ControllerInterface to ContainerInjectionInterface, no other changes.
Comment #65
disasm CreditAttribution: disasm commentedSince the text is static, moving drupal_set_title to the routing.yml _title attribute.
Comment #66
jibrandvt is converted to OO.
Comment #67
disasm CreditAttribution: disasm commentedAttached patch uses CsrfGenerator service. It also removes ContainerInjection since that comes along with ControllerBase. Finally, using configFactory instead of getting the specific config in the constructor.
Comment #69
disasm CreditAttribution: disasm commentedadding back ContainerInjectionInterface. ControllerBase doesn't implement it.
Comment #70
dawehnerYou don't have to inject though the config factory as there is a handy config() method on the ControllerBase
Comment #71
disasm CreditAttribution: disasm commentedAddressing #70.
Comment #73
dawehner#71: drupal8.system_theme_default.1987848-71.patch queued for re-testing.
Comment #75
disasm CreditAttribution: disasm commentedremoving config.factory from create().
Comment #76
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 #77
disasm CreditAttribution: disasm commentedFixing redirect() since it's path isn't converted to route yet.
Comment #78
dawehnerThank you very much!
Comment #79
alexpottNeed to remove function
system_theme_default()
from system.admin.inc - it's no longer used.Comment #80
star-szrAlso… nitpicks:
Extra * at the end of the docblock and the opening brace should not be on its own line. Summary line seems a bit terse and over-capitalized to me, how about "Gets the token generator service." ?
Comment #81
disasm CreditAttribution: disasm commentedProper dependency injection and addresses above comments.
Comment #82
disasm CreditAttribution: disasm commentedComment #83
disasm CreditAttribution: disasm commentedforgot to rebase first.
Comment #84
jibranwe should swap the order _constructor should be first method.
issue id please.
Comment #85
googletorp CreditAttribution: googletorp commented#83: drupal8.system_theme_default.1987848-83.patch queued for re-testing.
Comment #87
tlyngej CreditAttribution: tlyngej commentedAccording to #2089635: Convert non-test non-form page callbacks to routes and controllers I have changed the patch.
Comment #88
tlyngej CreditAttribution: tlyngej commentedComment #90
pratik60 CreditAttribution: pratik60 commented81: drupal8.system_theme_default.1987848-81.patch queued for re-testing.
Comment #92
pratik60 CreditAttribution: pratik60 commentedRerolling patch
Comment #93
pratik60 CreditAttribution: pratik60 commentedComment #95
pratik60 CreditAttribution: pratik60 commentedFixing the patch
Comment #97
pratik60 CreditAttribution: pratik60 commentedLatest patch with all necessary fixes
Comment #98
pratik60 CreditAttribution: pratik60 commentedComment #101
pratik60 CreditAttribution: pratik60 commented97: drupal8.system_theme_default.1987848-97.patch queued for re-testing.
Comment #102
pratik60 CreditAttribution: pratik60 commentedComment #103
alexpottNot sure that "patch to be ported" was the correct status - setting back to needs review.
Comment #104
dawehner97: drupal8.system_theme_default.1987848-97.patch queued for re-testing.
Comment #105
Crell CreditAttribution: Crell commentedControllers should no longer enforce CSRF tokens themselves. Instead, simply add:
_csrf_token: 'TRUE'
To the requirements section of the route name, then only use the generator to link to it rather than building the link yourself. The rest is automatic.
Is it still not converted?
Comment #106
Crell CreditAttribution: Crell commentedI hate the new issue queue workflow...
Comment #107
kim.pepperFixed the csrf tokens, the redirect response, and now using the ThemeHandlerInterface.
Comment #109
kim.pepperTesting this locally, the links set the default theme, and it displays on the site ok, but the active table in the Block Layout page is still bartik by default.
Comment #110
kim.pepperRe-roll
Comment #112
akozma CreditAttribution: akozma commentedComment #113
akozma CreditAttribution: akozma commentedRe-rolling the patch from #110.
I couldn't create an interdiff. After the rebase I added two small changes which were added later to system_theme_default() (this function is removed by the patch).
Adding the line Cache::deleteTags(array('local_task' => TRUE)); after the menu router has rebuild in \Drupal\system\Controller\ThemeController::defaultTheme
Comment #115
akozma CreditAttribution: akozma commentedRe-roll and update.
Comment #116
akozma CreditAttribution: akozma commentedComment #118
akozma CreditAttribution: akozma commented115: 1987848-system-theme-default-115.patch queued for re-testing.
Comment #120
akozma CreditAttribution: akozma commented115: 1987848-system-theme-default-115.patch queued for re-testing.
Comment #123
akozma CreditAttribution: akozma commentedComment #124
akozma CreditAttribution: akozma commentedComment #125
akozma CreditAttribution: akozma commentedRe-roll
Comment #126
akozma CreditAttribution: akozma commentedRe-roll
Comment #127
ParisLiakos CreditAttribution: ParisLiakos commentedthis cant work..there is no get method on $this
theme_handler service should be injected.
which means we have no test coverage there? :(
Comment #128
akozma CreditAttribution: akozma commentedtheme_handler injected
$this->get('theme_handler') changed to $this->themeHandler
list_themes() deprecated function changed to $this->themeHandler->listInfo();
Comment #129
Crell CreditAttribution: Crell commentedThis should be injected.
Otherwise this looks good visually.
Comment #130
kim.pepperThis injects the route.builder service.
Comment #131
ParisLiakos CreditAttribution: ParisLiakos commentedits
router.builder
notroute.builder
;)Comment #133
kim.pepperAhh! We should probably make that more consistent.
Comment #134
ParisLiakos CreditAttribution: ParisLiakos commentedagreed :)
Checked on simplytest, setting default theme works as should, code looks good => good to go! Thanks!
Comment #135
alexpottCommitted 972b532 and pushed to 8.x. Thanks!
Fixed during commit.