Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Convert this page callback to a new-style Controller, using the instructions on http://drupal.org/node/1800686
This may also require moving the theme functions from update.report.inc back to the .module file.
Comment | File | Size | Author |
---|---|---|---|
#22 | update-1938822-22.patch | 4.29 KB | YesCT |
#22 | interdiff-20-22.txt | 847 bytes | YesCT |
#20 | 18-20-interdiff.txt | 2.22 KB | alexpott |
#20 | update-1938822-20.patch | 4.29 KB | alexpott |
#18 | update-1938822-18.patch | 3.05 KB | tim.plunkett |
Comments
Comment #1
shnark CreditAttribution: shnark commentedComment #2
shnark CreditAttribution: shnark commentedComment #3
Crell CreditAttribution: Crell commentedFor a class, the @file directive is fairly simplistic. It's just the name of the class, like so:
@file
Contains \Drupal\update\Controller\UpdateController
(Yeah, it's silly, but that's what the coding standards say.)
Coding standards for methods are to use lowerCamel. So this should be:
public function updateStatus() {
Hm, this function. Actually, this can now be an injected service. Let's go ahead and convert that now, for practice. :-)
There's a service called module_handler, which can be injected via create(). Then there's a method on that service called loadInclude() that replaces this function.
Really, the contents of update.compare.inc should get turned into a service. That's a big complex, though, so we can do that in a follow-up.
Comment #4
shnark CreditAttribution: shnark commentedI made all the changes that Crell recommended in comment #3, except for the last one, which I will work on next.
Comment #5
YesCT CreditAttribution: YesCT commentedI think we can look at #1938296: Convert book_admin_overview and book_render to a new-style Controller again for an example.
etc.
look at this Manager bit for an example.
@Crell any more pointers than that?
---
moving it back to needs work to add the manager bit.
Comment #6
Crell CreditAttribution: Crell commentedThe book issue needed to make a new service. This issue doesn't. It just needs to use an existing service. So the only change would be in the create() method, where you have to reference the service we're injecting, and then replace module_load_include() with the equivalent from that service. There's no additional class like BookManager that would be needed.
Comment #7
YesCT CreditAttribution: YesCT commentedah! ok.
Comment #8
YesCT CreditAttribution: YesCT commented@EllaTheHarpy if you want to do this, just reassign it to yourself when you have time. Otherwise, this is up for grabs.
It's on my list to try, but I dont have time right away either... I'll check back in on this later.
Comment #9
sandhya.m CreditAttribution: sandhya.m commentedUsed moduleHandler to use loadInclude() for the inc files in UpdateController.php
Comment #10
dawehnerlet's order them.
Should be the interface of the module handler :)
Let's move it on top of create
I guess it's in the scope of this issue to return a build array.
Comment #11
alexpottTagging...
Comment #12
sandhya.m CreditAttribution: sandhya.m commentedChanges suggested by dawehner : done and added to the new patch
Comment #13
alexpotttagging
Comment #14
dawehnerShould be here marked as ModuleHandlerInterface as well.
You can use just {@inheritdoc}
... That's not a string, but a build array which is returned.
Comment #15
sandhya.m CreditAttribution: sandhya.m commentedUpdated the code as per the comments on #14
Comment #16
dawehnerGreat!
Comment #17
alexpottCommitted c7d74b3 and pushed to 8.x. Thanks!
@sandhya.m you need to set up your environment to make your patches a little easier to apply... see http://stackoverflow.com/questions/13675782/git-shell-in-windows-patchs-... / or you can use notepad++ to convert to utf-8 and unix line endings.
Comment #18
tim.plunkett#1953800: Make the database connection serializable means we cannot yet inject module_handler.
This was rolled back.
Comment #20
alexpottHmmm.... it's because update_theme relied on the menu to include update.report.inc :D
So the moduleHandler injection is fine... afaik the serialisation would only happen on a form conversion... when the controller gets added to form_state.
I have no idea how #15 passed... maybe because of the encoding testbot couldn't actually properly apply the patch but it thought it did.... very very weird.
Comment #21
tim.plunkettOh that makes a bit more sense. I just get twitchy when I see module_handler injected since I wasted a couple hours on it.
Comment #22
YesCT CreditAttribution: YesCT commentedI checked that the @param removed when Implements \... was replaced with {@inheritdoc} was not adding info.. and it was just the same @param that is in core/lib/Drupal/Core/ControllerInterface.php
So no loss of info.
just missing period.
The summary should be one line of up to 80 characters ending in ".".
http://drupal.org/node/1354#file
extra white space.
This looks rtbc to me. I compared it to what was done in #1938296: Convert book_admin_overview and book_render to a new-style Controller.
but I just changed some stuff, so someone else can rtbc it.
Comment #23
Crell CreditAttribution: Crell commentedLet's do.
Comment #24
alexpottCommitted a90da0d and pushed to 8.x. Thanks!