Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Comment | File | Size | Author |
---|---|---|---|
#69 | interdiff.txt | 3.08 KB | tim.plunkett |
#69 | update-2010246-69.patch | 51.8 KB | tim.plunkett |
Comment | File | Size | Author |
---|---|---|---|
#69 | interdiff.txt | 3.08 KB | tim.plunkett |
#69 | update-2010246-69.patch | 51.8 KB | tim.plunkett |
Comments
Comment #1
InternetDevels CreditAttribution: InternetDevels commentedComment #2
InternetDevels CreditAttribution: InternetDevels commentedPatch attached.
Comment #3
dawehnerShould be "Contains \..."
Should be {@inheritdoc}
Let's inject the settings into this class.
Empty line needed here.
hendler ^^. Let's call the variable moduleHandler and document it, to be a little more consistent.
This could be just {@inheritdoc}
Let's be consistent with the other file :) No it's not a db connection.
Let's open a follow up to move this to a library.
check_plain() can be replaced by String::checkPlain()
Isn't that the exact code of this validateForm method? I guess we can drop it
Just wondering why we can't use just #theme table + a prefix/weight set on that render element.
If you just call the parent method you can just drop the function completely.
That's a menu callback.
trailing whitespace
Comment #4
InternetDevels CreditAttribution: InternetDevels commentedThanks for your review. All issues fixed.
Comment #5
InternetDevels CreditAttribution: InternetDevels commentedForgot to put interdiff in the previous comment.
Comment #6
dawehnerThank you!
Just some small points left.
Use the moduleHandlerInterface in the method and document the $handler variable. It would be cool to be consistent with all other parts and name it $this->moduleHandler
Sorry. This has wrong indentation, a leftover whitespace and missing documentation.
I guess Drupal::state() should be used instead.
Nice!
I would say that this is more then just the settings array.
There should be in total three spaces before the "The"
Comment #7
InternetDevels CreditAttribution: InternetDevels commentedThanks for your new review. All issues fixed.
Comment #8
dawehnerComment #9
alexpottThis could be replaced with the injected module handler.
$archive_errors = $this->moduleHandler->invokeAll('verify_update_archive', array($project, $local_cache, $directory))
I can't see why this is required...
Same again... Is this needed?
Comment #10
InternetDevels CreditAttribution: InternetDevels commentedThanks for your new review. All issues are fixed.
The interdiff.txt was not added because old patch was not applied to new version of the Update module. A new patch was generated.
Comment #12
tim.plunkettThis blocks removal of MENU_LOCAL_ACTION.
Comment #13
tim.plunkettThis needed a big reroll, no interdiff was possible.
Comment #14
dawehner#13: update-2010246-13.patch queued for re-testing.
Comment #15
tim.plunkett#13: update-2010246-13.patch queued for re-testing.
Comment #17
plopescHello
Re-rolling, patch failed because the form theme function was removed from update.manager.inc and the Update page form was not displayed.
This patch removes this theme function and adds this functionality to the UpdateManagerUpdate class.
It should be green now.
Regards.
Comment #18
plopescRe-rolling this patch, once #2048933: Replace theme() with drupal_render() in update module has been committed :)
Regards.
Comment #19
tim.plunkettThis needs a reroll for FormBase
Comment #20
plopescRe-rolling.
Regards.
Comment #21
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 #22
disasm CreditAttribution: disasm commentedSome minor changes. After these are complete, I'm fine RTBC'ing this if tests pass, but we really need to get this into an update service (which should be opened as a follow-up). Note that that service would be used by more than just this, anything having to do with batch operations would use it as well.
lets get these methods in update.module as well.
can we move these functions to update.module?
if we move methods to update.module, we can get rid of this line.
$form['#title']
here as well
Comment #23
disasm CreditAttribution: disasm commentedComment #24
plopescAfter talk with @disasm and @timplunkett on IRC, decided not to move code to update.module.
This patch just get rid of drupal_set_title().
Regards.
Comment #25
jibranwe can use $request here.
Comment #26
googletorp CreditAttribution: googletorp commented#24: update-2010246-24.patch queued for re-testing.
Comment #28
googletorp CreditAttribution: googletorp commentedRerolled the patch since #2089635: Convert non-test non-form page callbacks to routes and controllers broke this patch.
I've used the . notation in the routing files which was introduced in the patch from above.
I've also add the removal of some of the code introduces in the above patch which no longer is needed.
Comment #29
pfrenssenLet's test it.
Comment #31
googletorp CreditAttribution: googletorp commented#28: update-2010246-28.patch queued for re-testing.
Comment #33
ACF CreditAttribution: ACF commented#28: update-2010246-28.patch queued for re-testing.
Re testing this as it was two failures with displayblocktest that failed, and checked locally and they seem to have just been random failures.
Comment #34
pfrenssenIt is not clear to me what this documentation means.
Constructs an UpdateManagerAccess object.
Comment #35
pfrenssenUpdated patch:
Comment #36
pfrenssenStraight reroll.
Comment #38
tim.plunkettReroll.
Comment #40
tkuldeep17 CreditAttribution: tkuldeep17 commentedWhen I applied patch in my local setup, I faced some errors. The errors are as following.
_update_manager_check_backends()
is not defined$container->get('state')
. This service returnsDrupal\Core\State\StateInterface
type object instead ofDrupal\Core\KeyValueStore\KeyValueStoreInterface
.For fixing it steps taken by me
_update_manager_check_backends()
in manager.update.incDrupal\Core\State\StateInterface
fromDrupal\Core\KeyValueStore\KeyValueStoreInterface
.Comment #42
tkuldeep17 CreditAttribution: tkuldeep17 commentedSorry same patch of tim.plunkett submitted.. Now submitting new patch.. :-)
Comment #43
tkuldeep17 CreditAttribution: tkuldeep17 commentedComment #45
tkuldeep17 CreditAttribution: tkuldeep17 commentedClass
Drupal\system\SystemConfigFormBase
does not exist. So I have changed toDrupal\Core\Form\FormBase
class for creating form in UpdateReady.php file.Comment #47
tim.plunkettThat should be \Drupal\Core\Form\ConfigFormBase, and use the $this->config() method instead of $this->configFactory->get().
Comment #48
tkuldeep17 CreditAttribution: tkuldeep17 commented@tim.plunkett
Thank you for reviewing patch. Fixed your point..
Comment #49
tkuldeep17 CreditAttribution: tkuldeep17 commentedChanged status..
Comment #51
tkuldeep17 CreditAttribution: tkuldeep17 commented@tim.plunkett
By seeing cause of failing my patch, it is failing in xmlrpc test. Why it is so.. need help...
Comment #52
tim.plunkettThere is no SystemConfigFormBase. It is \Drupal\Core\Form\ConfigFormBase, as I said before.
And it doesn't need the config context.
Comment #53
tkuldeep17 CreditAttribution: tkuldeep17 commentedSorry, I created path with old code. Now uploading updated patch.
Comment #54
tkuldeep17 CreditAttribution: tkuldeep17 commentedComment #56
tkuldeep17 CreditAttribution: tkuldeep17 commentedVery Sorry for repeatedly uploading wrong patch.. Uploading correct patch..
Comment #58
tkuldeep17 CreditAttribution: tkuldeep17 commentedFixes exception..
Comment #60
tim.plunkettRerolled.
Comment #61
tim.plunkettForgot to remove dead code.
Comment #64
tim.plunkettWe removed the theme function, we have to remove the definition.
Comment #65
tim.plunkettGit mixup. Interdiff was right.
Comment #67
jibranIt's green. I haven't found anything coding wise so RTBC.
Comment #68
alexpottDeprecated function use.
Unnecessary variables.
Deprecated function use.
Unused uses.
Comment #69
tim.plunkettFair points. My interdiff matches your review.
Comment #70
alexpottCommitted 8f94da5 and pushed to 8.x. Thanks!