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

Files: 
CommentFileSizeAuthor
#34 1957154-module_handler.patch31.86 KBayelet_Cr
PASSED: [[SimpleTest]]: [MySQL] 55,848 pass(es).
[ View ]
#31 1957154-module_handler.patch31.87 KBayelet_Cr
PASSED: [[SimpleTest]]: [MySQL] 55,812 pass(es).
[ View ]
#28 1957154-module_handler.patch31.87 KBayelet_Cr
FAILED: [[SimpleTest]]: [MySQL] 55,723 pass(es), 16 fail(s), and 0 exception(s).
[ View ]
#18 interdiff.txt4.67 KBjoates
#17 drupalcore-rerolled-replace-calls-to-module_handler-1957154-17.patch31.82 KBjoates
FAILED: [[SimpleTest]]: [MySQL] 54,464 pass(es), 15 fail(s), and 0 exception(s).
[ View ]
#11 module_handler-1957154-11.patch32.45 KBjoates
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch module_handler-1957154-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#7 module_handler-1957148-7.patch32.45 KBjoates
FAILED: [[SimpleTest]]: [MySQL] 54,232 pass(es), 15 fail(s), and 0 exception(s).
[ View ]
#4 module_handler-1957148-4.patch32.47 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch module_handler-1957148-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

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

Assigned:Unassigned» xjm

Beta-testing these issues.

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.

Status:Active» Postponed
StatusFileSize
new32.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch module_handler-1957148-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Postponed» Needs review

Status:Needs review» Needs work

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

Status:Needs review» Needs work
StatusFileSize
new32.45 KB
FAILED: [[SimpleTest]]: [MySQL] 54,232 pass(es), 15 fail(s), and 0 exception(s).
[ View ]

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');

Status:Needs work» Needs review

Issue tags:-Novice

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

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

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

StatusFileSize
new32.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch module_handler-1957154-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

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

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.

Status:Needs work» Needs review
StatusFileSize
new31.82 KB
FAILED: [[SimpleTest]]: [MySQL] 54,464 pass(es), 15 fail(s), and 0 exception(s).
[ View ]

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

StatusFileSize
new4.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)

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)

Status:Needs work» Needs review

needs review if it passes the tests this time.

Status:Needs review» Needs work

Assigned:Unassigned» joates

i'm debugging the tests locally..

StatusFileSize
new14.87 KB
new24.75 KB

the first test passes locally !!

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

local pass
test-1-local-pass.png

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

StatusFileSize
new16.26 KB
new11.58 KB

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

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

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

Status:Needs work» Needs review
StatusFileSize
new31.87 KB
FAILED: [[SimpleTest]]: [MySQL] 55,723 pass(es), 16 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

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

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.

Status:Needs work» Needs review
StatusFileSize
new31.87 KB
PASSED: [[SimpleTest]]: [MySQL] 55,812 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Nice and straightforward. Thanks!

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

Status:Needs work» Needs review
StatusFileSize
new31.86 KB
PASSED: [[SimpleTest]]: [MySQL] 55,848 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

And again.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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

Issue summary:View changes

Updated issue summary with current status message.