Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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 |
---|---|---|---|
#76 | interdiff.txt | 15.06 KB | kim.pepper |
#76 | 1987888-update-manual-status-76.patch | 49.28 KB | kim.pepper |
#74 | interdiff.txt | 11.36 KB | kim.pepper |
#74 | 1987888-update-manual-status-74.patch | 50.18 KB | kim.pepper |
#70 | 1987888-update-manual-status-70.patch | 43.79 KB | kim.pepper |
Comments
Comment #1
zengenuity CreditAttribution: zengenuity commentedTaking a swing at this.
Comment #2
zengenuity CreditAttribution: zengenuity commentedThe attached patch reworks
update_manual_status()
to use the new style controller. The code from update.fetch.inc is now found in UpdateFetchController.php and UpdateFetchManager.php. In addition, I started the process of converting update.compare.inc to UpdateCompareManager.php so that I could use dependency injection for the one related function from update.compare.inc. I left the rest update.compare.inc in place, since I wasn't sure if I'm doing this 100% correctly. (this is my first WSCCI controller conversion)In the process of doing this conversion, I ran into a question about how to pass methods from the new controllers to
batch_set()
. It appears that using something like\Drupal\update\Controller\UpdateFetchController::updateFetchDataBatch
works in the operations key, but not in the finished key. So, I had to move the finished function into update.module. (since I've deleted update.fetch.inc) This is probably not the best solution here, but I didn't want to start messing around withbatch_finished()
. (There is afunction_exists()
call that doesn't work with the new style path and prevents the finished callback from firing if it's in the new style.)This is my first WSCCI controller conversion, so feedback is appreciated. Not 100% sure I'm doing this right, but it does seem to work.
Comment #3
zengenuity CreditAttribution: zengenuity commentedChanging status to Needs Review
Comment #5
zengenuity CreditAttribution: zengenuity commentedBad formatting of patch. Let's try this one.
Comment #7
zengenuity CreditAttribution: zengenuity commentedSome issues with the tests themselves. Running this though testbot one more time before heading to the code sprint.
Comment #9
zengenuity CreditAttribution: zengenuity commentedTrying this again.
Comment #11
vijaycs85Re-roll with minor code and interface fixes...
Comment #13
zengenuity CreditAttribution: zengenuity commented#11: 1987888-update-manual-status-11.patch queued for re-testing.
Comment #15
zengenuity CreditAttribution: zengenuity commentedFixed the tests that failed last time. Let's see how tests go this time.
Comment #16
dawehnerGood work so far!
I guess this comment can be dropped because the information on there is not really helpful
Please remove the 'Page callback' in front, that's an old c&p of the documentation.
You should inject the update.fetch service.
I'm wondering whether batch supports callables (so an instance of the object) as well.
Because then you could inject the queue service in there as well.
This is really cool and allows maybe one day a proper unit test.
Wouldn't it be possible to drop the setUp method in total here?
So let's drop these lines
See first comment.
Should be @return array. Please have a look at all the other docs as they need some small polishing as well.
I guess this is a public method?
instead of using statics you should use properties on the object instead.
Please inject the various dependencies like the config factory, the module handler and the key value expirable service.
more dependencies
Yeah another codeblock with a lot of dependencies and static variables, see above.
Comment #17
zengenuity CreditAttribution: zengenuity commentedI've tried to fix all the issues mentioned by @dawehner. The only one I got totally stumped on was the batch and callables. I've never done that before, but after some Googling I had it working until I added the dependency injection stuff, which broke it.
What I found is that this would work, with a non-static fetchDataBatch method:
But, once I started injecting the UpdateFetchManager, it needs to be something like this:
Whenever I ran it with the constructor parameter in there, I got an error:
Fatal error: Call to a member function prepare() on a non-object in /DRUPAL_ROOT/core/lib/Drupal/Core/Database/Connection.php on line 339
Was not sure what to do at that point, so I switched it back to the static method. I may have been totally off-base on this, though. So, if you can show me an example of what you mean, I will give it a try. BTW, the "finished" callback of the batch doesn't seem to support any of the new OO stuff at all. It has a
function_exists()
check in it that breaks both the static and non-static method callback.Comment #19
zengenuity CreditAttribution: zengenuity commentedLocale module calls functions in update.compare.inc that have been migrated to UpdateCompareManager.php. I've switched the locale code to use the new service.
Comment #20
dawehnerCan't you maybe pass in a full callable, so you can have a proper object with injected services? ... Let's typehint $context as array.
Wow this is sad!
This should be all ModuleHandlerInterface
Would be cool to have the full namespace in there.
Just an idea, we could store the actual instance on the object and not the factory.
Let's typehint as array
It is probably a good follow up to move this maybe on this class.
$key is probably a string.
Another advantage of storing the factory would be that there is no requirement to ask for the instance all the time.
drupal_map_assoc can be replaced by MapArray::copyValuesToKeys
I suggest to do the same thing as in the other class for all this different factories.
So the http client should be injected.
Seriously, guzzle rocks!
Comment #21
zengenuity CreditAttribution: zengenuity commentedOkay, I've tried to address all the issues from #20: updating type hinting throughout these classes, switching over to storing the objects rather than the factories, and injecting the httpClient.
A couple things I wasn't able to do:
The callable for the batch callback. I don't know what to do here, but that's because I haven't passed a callable like this before. So, if someone can point me to an example, I will try to implement this. I did try a couple things described in #17, so check the note there about the problem I ran into.
The switch from UnitTestBase to DrupalUnitTestBase: I had to do this because dependency injection code doesn't work in UnitTestBase. Perhaps the tests should be rewritten to not depend on that, but again, I'm not sure what do here. UpdateFetchManager depends on a bunch of services, and I'm not sure I'm familiar enough with them to create them independently without getting them from the container. If there's an example somewhere, that would be helpful.
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedComment #23
zengenuity CreditAttribution: zengenuity commentedRerolled.
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedrelated #2002116: Convert core/modules/update/lib/Drupal/update/Tests/UpdateCoreUnitTest.php to phpunit
Comment #25
dipen chaudhary CreditAttribution: dipen chaudhary commentedNeeds to be rerolled, patch didn't apply.
Comment #26
zengenuity CreditAttribution: zengenuity commentedRerolled.
Comment #27
zengenuity CreditAttribution: zengenuity commentedComment #28
star-szrThanks for sticking with this @zengenuity! Needs another reroll.
Comment #29
vijaycs85Re-roll...
Comment #30
Crell CreditAttribution: Crell commentedThis looks like a great candidate for a PHPUnit test instead.
Same. PHPUnit FTW.
Translation should be injected.
This number of dependencies makes me wonder if this class shouldn't be broken up into separate pieces in the first place. It's feeling somewhat God-ish.
With all the refactoring going on here anyway, let's not leave this procedural legacy around as it makes the class un-unit-testable. (Or at least not without hackery.)
Comment #31
Crell CreditAttribution: Crell commentedComment #32
zengenuity CreditAttribution: zengenuity commentedI re-rolled the patch and tried to address a couple of the points made by @Crell. I could use some direction on some of these:
1 & 2: I don't know exactly how to proceed with changing these tests to PHPUnit. Is there an example somewhere I can look at of a test that was converted?
3: Translation is now injected.
4: God-ish UpdateFetchManager: Yes, agreed, but I'm not experienced enough with core dev to determine how to break it up. If someone can give me a plan for it, I'm willing to try to break up the code.
5: Procedural code in UpdateCompareManager: So, I started this process. I've removed the two lines noted by @Crell, but to do so I needed to move procedural code into the UpdateCompareManager & UpdateFetchManager classes. That's probably okay, except now I have even more procedural functions called from the functions I moved. Some of these can be injected, I suspect, like whatever the replacements for l() and url() are. But, I decided to stop before I end up making another God-ish class with long list of injected classes. Finishing this up may depend on what is decided for #4.
Comment #33
zengenuity CreditAttribution: zengenuity commentedComment #35
Crell CreditAttribution: Crell commentedLet's get dww's take on the refactoring here. (And maybe what parts should be punted.)
Comment #36
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 #37
zengenuity CreditAttribution: zengenuity commentedHere's the minimal patch that just adds the route definition and controller class method.
Comment #39
disasm CreditAttribution: disasm commentedThanks for jumping on this so quickly @zengenuity. That's not quite what we're doing with these though. Here's a patch with an interdiff so you can see what's being done.
Comment #40
xjmThanks @disasm! Let's also file a followup issue right away with the diff of the previous patch in #32 against this one so that the previous work is still there to iterate on later.
Edit: Just saw the bit in the summary about resetting the issue status to active. I'm not sure who added that (the revision log is tricksy) but I don't think that's the right tactic. We need to be able to tell when the conversions have been committed.
Comment #41
dawehnerSo yeah, let's do the full thing later.
This patch just removes the hook_menu entry completely.
Comment #43
dawehner#41: update-1987888-41.patch queued for re-testing.
Comment #45
jibranDisplayBlockTest fails again i am looking at you @tim.plunkett
Comment #46
jibran#41: update-1987888-41.patch queued for re-testing.
Comment #47
andypost+1 RTBC. Just a minor nitpicks:
needs wrapping around 80 chars
seems better to include the file itself without module_load_include() __DIR__ . '..\..
Comment #48
_12345678912345678 CreditAttribution: _12345678912345678 commented#41: update-1987888-41.patch queued for re-testing.
Comment #50
_12345678912345678 CreditAttribution: _12345678912345678 commentedFailed when I re-tested it. :(
Comment #51
zengenuity CreditAttribution: zengenuity commentedThe simple route conversion were already committed as part of this commit: http://drupalcode.org/project/drupal.git/commit/ef2e45b0e86fc6863b8ee6d3...
Was the follow-up refactoring issue created?
Comment #52
ParisLiakos CreditAttribution: ParisLiakos commentedlets keep it up here.#32 needs a reroll
Comment #53
JB13 CreditAttribution: JB13 commentedrerolling the patch.
Comment #54
pfrenssenSome minor cleanups:
Comment exceeds 80 characters.
This adds an empty line to the end of the file.
Comment #55
disasm CreditAttribution: disasm commentedquick updates are in, reroll of #29. Bound to be some issues with everything that's happened in the last month, but this'll at least jump start this issue again.
Comment #56.0
(not verified) CreditAttribution: commentedadding notification that this is a stub.
Comment #57
zengenuity CreditAttribution: zengenuity commentedHere's another pass at the refactoring task. (the stubs in the previous patches are already in)
This patch breaks the functions required for this callback into three classes to minimize external dependencies for each one:
UpdateFetcher: somebody else wrote this for another issue. It just wraps around Guzzle to fetch a single project's info. depends on ClientInterface
UpdateProcessor: does all the fetch queuing and processing the full list of projects. depends on UpdateFetcher and QueueFactory
UpdateManager: other functions that don't directly require the queue or fetcher. depends on UpdateProcessor and ModuleHandler
Comment #59
zengenuity CreditAttribution: zengenuity commentedForgot to include the new files in the patch. Re-testing.
Comment #60
kim.pepper59: 1987888-update-manual-status-59.patch queued for re-testing.
Comment #61
Crell CreditAttribution: Crell commentedThese should be injected, too.
As above. Inject these.
It looks like this is a lot of dependencies in this class. That's a hint that it may need to get split up into different objects.
Inject this. \Drupal should *never* appear inside a class.
This issue has basically morphed into "objectify the update system". Should it be retitled?
Comment #62
kim.pepperI'll have a go at this.
Comment #63
kim.pepperFixes procedural calls to \Drupal-> by injecting some more services. Also a couple of docs clean ups.
Comment #64
Crell CreditAttribution: Crell commentedThat's much better, thanks.
Given the scope of this issue, I think Derek should weigh in before it gets RTBCed. Derek, any feedback before this is commitable?
Comment #65
dwwI'll have to find someone in IRC who can explain to me what a "new style controller" means. I'm so far out of the loop on D8 I don't have any frame of reference for even starting to review this patch. I mean, I can go through it like an ignorant monkey and point out WTFs like this:
Why do we sometimes separate with a _ and other times with a .?
But I doubt that's the kind of review you're looking for. There's nothing in the summary to tell me what problem this patch is trying to solve, what thing could be accomplished once it was committed that's not possible now, etc.
Thanks for asking, though. :) But, if anyone's more familiar with D8 and what's happening wants to RTBC, please do. No D8 core issue should be held up waiting my blessings at this point...
Cheers,
-Derek
Comment #66
zengenuity CreditAttribution: zengenuity commentedAssigning this back to myself because I'm willing to keep working on this. I know there needs to be a second pass through update.module, update.fetch.inc and update.compare.inc to refactor some additional functions that could end up using the new classes, but I posted my latest patch hoping for some feedback about whether my overall approach was reasonable before moving forward.
This is my first significant core patch, so I'm not on top of what everyone else is doing and whether the changes in this patch are going to be made redundant by somebody else's work. I'm happy to keep working on this, but I just don't want to end up wasting time if it can't be used.
Comment #67
Crell CreditAttribution: Crell commented63: 1987888-update-manual-status-63.patch queued for re-testing.
Comment #70
kim.pepperRe-roll
Comment #71
Crell CreditAttribution: Crell commentedAside from the need to reroll, I was fairly OK with where this patch was in #63. I just wanted someone who knew the update system to verify it. I guess #65 is as much as we're going to get, though.
I suggest we get this committed, and then we can open a normal follow-up to break up the classes even more if someone is interested in doing so. That's not release-blocking, though, and IMO not an API change so it's not pressing.
Comment #72
Crell CreditAttribution: Crell commentedFollow-up filed: #2167779: Break up Update system classes Thanks, everyone!
zengenuity: If you're interested, you can have dibs on the follow-up issue. I'm fairly certain no one else is working on it yet since I just filed it. :-)
Comment #73
ParisLiakos CreditAttribution: ParisLiakos commentedthose classes should get interfaces at least
Comment #74
kim.pepperExtracted interfaces for UpdateProcessor and UpdateFetcher.
Comment #75
ParisLiakos CreditAttribution: ParisLiakos commentedThanks! :)
oh fetcher didnt have an interface as well..but we still need one for manager ;)
we can and should use inheritdocs here now
Comment #76
kim.pepperThanks for the review Paris.
Here's a patch which fixes those issues. I also fixed a bit of docblock formatting.
Comment #77
ParisLiakos CreditAttribution: ParisLiakos commentedcool, thanks, interdiff looks good.
lets get this in and move on #2167779: Break up Update system classes
Comment #78
alexpottCommitted cc1892d and pushed to 8.x. Thanks!
Noticed #2181101: Update settings tab not visible during manual testing.
Fixed a small spelling mistake during commit.