Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Follow-up of #1937600: Determine what services to register in the new Drupal class
Replace all calls to the module_handler service with Drupal::moduleHandler().
Current Status:
further progress is currently being delayed because the REST endpoint on the testbot is not responding (returns HTTP 500) and therefore @xjm's patch cannot pass all the tests, although when running the tests locally they are passing without errors !!
Comment | File | Size | Author |
---|---|---|---|
#34 | 1957154-module_handler.patch | 31.86 KB | ayelet_Cr |
#31 | 1957154-module_handler.patch | 31.87 KB | ayelet_Cr |
#28 | 1957154-module_handler.patch | 31.87 KB | ayelet_Cr |
#18 | interdiff.txt | 4.67 KB | joates |
#17 | drupalcore-rerolled-replace-calls-to-module_handler-1957154-17.patch | 31.82 KB | joates |
Comments
Comment #1
BerdirInstructions:
Search & replace all calls to drupal_container()->get('module_handler') or Drupal::service('module_handler') with the mentioned method. Note that calls within namespaced classes (have a namespace line and are in the lib/ folder) need to be "\Drupal::...".
Comment #2
xjmBeta-testing these issues.
Comment #3
xjmThere's a slightly more advanced task related to these issues.
TestBase
should doalready does a\Drupal::setContainer($this->container)
. So, instances of$this->container->get('servicename')
could also be cleaned up in tests. Skipping them for now to stick with the program.Comment #4
xjmStatisticsSettingsForm
takes an injected module handler and container, interestingly.Not sure about the changes in DUTB; I have a sneaking suspicion there might have been an "intended" behavior here of using
drupal_container()
to get the original container rather than the test environment's. Ditto ModuleApiTest, ModulesDisabledUpgradePathTest, and UpgradePathTestBase. We might need to update the instructions to skip tests on a first pass and do them separately.Comment #5
BerdirComment #7
joates CreditAttribution: joates commentedminor change because of..
Comment #8
joates CreditAttribution: joates commentedComment #10
joates CreditAttribution: joates commented#7: module_handler-1957148-7.patch queued for re-testing.
Comment #11
joates CreditAttribution: joates commentednoticed this patch has incorrect issue number in filename.. changed that
Comment #13
xjmThanks @joates for your help!
A couple tips. This issue was currently assigned to me. If you want to work on a patch for an issue that's assigned to someone, it's usually a good idea to check with that person first--either in IRC or on the issue--to make sure they aren't working on it, so that you don't duplicate effort.
Also, please provide an interdiff when updating patches (yours or someone else's). This makes it easy for reviewers and the original patch authors to see the changes you are making. See: http://drupal.org/node/1488712
The next step for this issue is to debug the test failures. Clicking the "view details" link on the patch will show what tests are failing, and then the tests can be run in a local installation for debugging purposes.
Comment #14
joates CreditAttribution: joates commented@xjm apologies for that..
i was only trying to help it pass the tests with that 1 small change, i edited the patch file (because the conflict was not on a line that you had changed but on another line that needs to match so that the changed line can be located correctly -- i guess?) so i wasn't exactly sure how to "interdiff.txt" that workflow step, i *did* document the change in the comment :)
the 15 failed tests are
ALLmostly HTTP 500 Internal Server Errors as far as i can see from the reports.Comment #15
joates CreditAttribution: joates commented#11: module_handler-1957154-11.patch queued for re-testing.
Comment #17
joates CreditAttribution: joates commentedrerolled the patch from #4 (there was only 1 conflict to resolve)
@xjm sorry again!
i probably messed things up in #7 by not doing it correctly -- my bad :((
uploading patch file..
(i have read xjm's blog post but i still don't know how to do an interdiff.txt when resolving a merge conflict -- because i cannot diff against a patch that won't apply !!??)
Comment #18
joates CreditAttribution: joates commentedfound that i can do this instead..
interdiff old.patch new.patch > ~/interdiff.txt
(edit: mainly these are just changes of the line numbers)
Comment #19
joates CreditAttribution: joates commentedi had to remove this part to resolve the conflict.. the function
field_sync_field_status()
now resides at line 398 in the .module file and it looks like somebody else has already implemented the change on line 399 with this line:$module_handler = Drupal::moduleHandler();
($module_handler is then used throughout the rest of the function)
Comment #20
joates CreditAttribution: joates commentedneeds review if it passes the tests this time.
Comment #22
joates CreditAttribution: joates commentedi'm debugging the tests locally..
Comment #23
joates CreditAttribution: joates commentedthe first test passes locally !!
testbot-fail
local pass
Comment #24
joates CreditAttribution: joates commentedcore/modules/rest/lib/Drupal/rest/Tests/CreateTest.php lines 52-54:
is the REST interface enabled on the testbot server (qa.drupal.org) ???
Comment #25
joates CreditAttribution: joates commentedthe second test also passes locally !!
testbot fail
local pass
test code
again the REST endpoint borks !! :))
and returns 500
Comment #26
joates CreditAttribution: joates commentedmaybe someone from the #WSCCI team can get involved at this point ?
Comment #27
Crell CreditAttribution: Crell commentedComment #28
ayelet_Cr CreditAttribution: ayelet_Cr commentedComment #30
BerdirFatal error: Class 'Drupal\Core\Entity\Field\Type\Drupal' not found in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Entity/Field/Type/Field.php on line 158
That's a relatively obvious one, that should be a \Drupal in that file and not Drupal.
Not sure about the other fails.
Comment #31
ayelet_Cr CreditAttribution: ayelet_Cr commentedComment #32
Crell CreditAttribution: Crell commentedNice and straightforward. Thanks!
Comment #33
alexpottPatch does not apply...
Comment #34
ayelet_Cr CreditAttribution: ayelet_Cr commentedComment #35
Crell CreditAttribution: Crell commentedAnd again.
Comment #36
webchickCommitted and pushed to 8.x. Thanks!
Comment #37.0
(not verified) CreditAttribution: commentedUpdated issue summary with current status message.