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 !!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Issue tags: +Novice

Instructions:

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::...".

xjm’s picture

Assigned: Unassigned » xjm

Beta-testing these issues.

xjm’s picture

There's a slightly more advanced task related to these issues. TestBase should do already 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.

xjm’s picture

Status: Active » Postponed
FileSize
32.47 KB

StatisticsSettingsForm 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.

Berdir’s picture

Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, module_handler-1957148-4.patch, failed testing.

joates’s picture

Status: Needs review » Needs work
FileSize
32.45 KB

minor change because of..

error: patch failed: core/includes/module.inc:287
error: core/includes/module.inc: patch does not apply
-  $schema_store = drupal_container()->get('keyvalue')->get('system.schema');
+  $schema_store = Drupal::keyValue('system.schema');
joates’s picture

Status: Needs work » Needs review

Issue tags: -Novice

The last submitted patch, module_handler-1957148-7.patch, failed testing.

joates’s picture

Status: Needs work » Needs review
Issue tags: +Novice

#7: module_handler-1957148-7.patch queued for re-testing.

joates’s picture

noticed this patch has incorrect issue number in filename.. changed that

Status: Needs review » Needs work

The last submitted patch, module_handler-1957154-11.patch, failed testing.

xjm’s picture

Assigned: xjm » Unassigned

Thanks @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.

joates’s picture

@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 ALL mostly HTTP 500 Internal Server Errors as far as i can see from the reports.

joates’s picture

Status: Needs work » Needs review
Issue tags: -Novice

#11: module_handler-1957154-11.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice

The last submitted patch, module_handler-1957154-11.patch, failed testing.

joates’s picture

Status: Needs work » Needs review
FileSize
31.82 KB

rerolled 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 !!??)

joates’s picture

FileSize
4.67 KB

found that i can do this instead..

interdiff old.patch new.patch > ~/interdiff.txt

(edit: mainly these are just changes of the line numbers)

joates’s picture

Status: Needs review » Needs work
+++ a/core/modules/field/field.moduleundefined
@@ -533,7 +533,7 @@
 function field_sync_field_status() {
   // Refresh the 'active' and 'storage_active' columns according to the current
   // set of enabled modules.
+  $modules = array_keys(drupal_container()->get('module_handler')->getModuleList());
-  $modules = array_keys(Drupal::moduleHandler()->getModuleList());
   foreach ($modules as $module_name) {
     field_associate_fields($module_name);

i 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)

joates’s picture

Status: Needs work » Needs review

needs review if it passes the tests this time.

Status: Needs review » Needs work
joates’s picture

Assigned: Unassigned » joates

i'm debugging the tests locally..

joates’s picture

the first test passes locally !!

testbot-fail
test-1-testbot-fail.png

local pass
test-1-local-pass.png

joates’s picture

Assigned: joates » Unassigned

core/modules/rest/lib/Drupal/rest/Tests/CreateTest.php lines 52-54:

    // Create the entity over the REST API.
    $this->httpRequest('entity/' . $entity_type, 'POST', $serialized, $this->defaultMimeType);
    $this->assertResponse(201);

is the REST interface enabled on the testbot server (qa.drupal.org) ???

joates’s picture

the second test also passes locally !!

testbot fail
test-2-testbot-fail.png

local pass
test-2-local-pass.png

test code

      // Make sure that field level access works and that the according field is
      // not available in the response.
      // @see entity_test_entity_field_access()
      $entity->field_test_text->value = 'no access value';
      $entity->save();
      $response = $this->httpRequest('entity/' . $entity_type . '/' . $entity->id(), 'GET', NULL, $this->defaultMimeType);
      $this->assertResponse(200);

again the REST endpoint borks !! :))
and returns 500

joates’s picture

maybe someone from the #WSCCI team can get involved at this point ?

Crell’s picture

Title: Replace calls to module_handler service with Drupal::moduleHandler() » Replace calls to drupal_container()->get('module_handler') service with Drupal::moduleHandler()
Issue tags: +WSCCI
ayelet_Cr’s picture

Status: Needs work » Needs review
FileSize
31.87 KB

Status: Needs review » Needs work

The last submitted patch, 1957154-module_handler.patch, failed testing.

Berdir’s picture

Fatal 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.

ayelet_Cr’s picture

Status: Needs work » Needs review
FileSize
31.87 KB
Crell’s picture

Status: Needs review » Reviewed & tested by the community

Nice and straightforward. Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Patch does not apply...

curl https://drupal.org/files/1957154-module_handler_0.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 32636  100 32636    0     0  24191      0  0:00:01  0:00:01 --:--:-- 30107
error: patch failed: core/includes/module.inc:289
error: core/includes/module.inc: patch does not apply
ayelet_Cr’s picture

Status: Needs work » Needs review
FileSize
31.86 KB
Crell’s picture

Status: Needs review » Reviewed & tested by the community

And again.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary with current status message.