Comments

Issue tags:+Novice

Tagging

Assigned:Unassigned» Sam152

I can take a look at this over the weekend. Keen to start looking into some core development.

StatusFileSize
new58.19 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

I have run through all the instances of module_exists and replaced with the updated method. I have also removed the deprecated function as per Crells recommendation. It didn't seem to break anything, hopefully it can pass the testbot so it doesn't have to be rerolled too many times.

Status:Active» Needs review

Not sure if we are removing the functions at this point. @catch mention in IRC we should leave them.

Updated status for test bot.

Status:Needs review» Active
StatusFileSize
new57.68 KB
FAILED: [[SimpleTest]]: [MySQL] 44,091 pass(es), 375 fail(s), and 70 exception(s).
[ View ]

Here is a version without the deprecated function removed. Either one should work depending on the final decision made in this issue.

Status:Active» Needs review

I noticed a few places where \Drupal had not been 'use'd, so I will wait for the test results to come back negative and then fix up the individual cases when they arise.

I need to setup the test suite locally.

Status:Needs review» Needs work

The last submitted patch, drupal-2045931-replace-module-exists-calls-4.patch, failed testing.

StatusFileSize
new57.51 KB
PASSED: [[SimpleTest]]: [MySQL] 57,148 pass(es).
[ View ]

This patch makes appropriate use of \Drupal where required and includes the deprecated function as per tim.plunkett's suggestion.

Status:Needs work» Needs review

Status:Needs review» Needs work

see https://drupal.org/node/1353118 for when to use absolute vs relative. It seems if the class is global and the file has a namespace it should be absolute. However if the file does not have a namespace it should be relative.

Thanks for link. I believe the patch conforms to the standard after reading the docs.

This patch no longer applies. Any chance of a re-roll.

Yeah, I will manually repeat the work required for this patch as I doubt re-rolling would be very clean either. Will repost and hit up someone on IRC to have it applied quickly after it's creation.

StatusFileSize
new58.32 KB
PASSED: [[SimpleTest]]: [MySQL] 57,504 pass(es).
[ View ]

I have attached another patch. Would be good to get this committed as soon as it passes testing to prevent further duplication of work.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

Looks good, let's try get this in before it needs another re-roll.

Status:Reviewed & tested by the community» Needs work

The current patch replaces module_exists() with \Drupal::moduleHandler.

I suggest that this patch broken into two. One part to deal with the procedural code (everything not in a /lib/... directory. And another to do the OO code correctly.

All the changes to procedural code in this patch look great!

What follows is a review of the OO code.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/InfoAlterTest.phpundefined
@@ -32,7 +32,7 @@ function testSystemInfoAlter() {
-    $this->assertTrue(module_exists('module_test'), 'Test module is enabled.');
+    $this->assertTrue(\Drupal::moduleHandler()->moduleExists('module_test'), 'Test module is enabled.');
@@ -47,7 +47,7 @@ function testSystemInfoAlter() {
-    $this->assertFalse(module_exists('module_test'), 'Test module is disabled.');
+    $this->assertFalse(\Drupal::moduleHandler()->moduleExists('module_test'), 'Test module is disabled.');

This is just one example from the patch... there are so many more I did not bother to list them all. In tests the simpletest runner has access to the container and therefore we can use $this->container->get('module_handler') instead of \Drupal::moduleHandler().

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockTypeFormController.phpundefined
@@ -53,7 +53,7 @@ public function form(array $form, array &$form_state) {
-    if (module_exists('content_translation')) {
+    if (\Drupal::moduleHandler()->moduleExists('content_translation')) {
+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockTypeListController.phpundefined
@@ -20,7 +20,7 @@ class CustomBlockTypeListController extends ConfigEntityListController {
-    if (module_exists('field_ui')) {
+    if (\Drupal::moduleHandler()->moduleExists('field_ui')) {
+++ b/core/modules/block/lib/Drupal/block/BlockAccessController.phpundefined
@@ -73,7 +73,7 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
-      elseif (module_exists('php')) {
+      elseif (\Drupal::moduleHandler()->moduleExists('php')) {
+++ b/core/modules/block/lib/Drupal/block/BlockFormController.phpundefined
@@ -85,7 +85,7 @@ public function form(array $form, array &$form_state) {
-      if (module_exists('php') && $access) {
+      if (\Drupal::moduleHandler()->moduleExists('php') && $access) {
@@ -108,7 +108,7 @@ public function form(array $form, array &$form_state) {
-    if (module_exists('language') && language_multilingual()) {
+    if (\Drupal::moduleHandler()->moduleExists('language') && language_multilingual()) {
+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestFormController.phpundefined
@@ -47,7 +47,7 @@ public function form(array $form, array &$form_state) {
-    if (module_exists('image')) {
+    if (\Drupal::moduleHandler()->moduleExists('image')) {
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.phpundefined
@@ -43,17 +43,17 @@ public function form(array $form, array &$form_state) {
-    if (module_exists('language')) {
+    if (\Drupal::moduleHandler()->moduleExists('language')) {
@@ -86,14 +86,14 @@ protected function actions(array $form, array &$form_state) {
-      if (module_exists('language')) {
+      if (\Drupal::moduleHandler()->moduleExists('language')) {
...
-      if (module_exists('content_translation')) {
+      if (\Drupal::moduleHandler()->moduleExists('content_translation')) {

These can be injected by implementing EntityControllerInterface and adding a createInstance method. For an example have a look at \Drupal\action\ActionAddFormController

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityNormalizer.phpundefined
@@ -80,7 +80,7 @@ public function denormalize($data, $class, $format = NULL, array $context = arra
-    elseif (module_exists('language')) {
+    elseif (\Drupal::moduleHandler()->moduleExists('language')) {

This should be injected by updating serializer.normalizer.entity.hal definition in hal.services.yml perhaps using setter injection.

+++ b/core/modules/history/lib/Drupal/history/Plugin/views/field/HistoryUserTimestamp.phpundefined
@@ -34,7 +34,7 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
-      if (module_exists('comment') && !empty($this->options['comments'])) {
+      if (\Drupal::moduleHandler()->moduleExists('comment') && !empty($this->options['comments'])) {
@@ -50,7 +50,7 @@ protected function defineOptions() {
-    if (module_exists('comment')) {
+    if (\Drupal::moduleHandler()->moduleExists('comment')) {
@@ -78,7 +78,7 @@ public function render($values) {
-      $last_comment = module_exists('comment') && !empty($this->options['comments']) ?  $this->getValue($values, 'last_comment') : 0;
+      $last_comment = \Drupal::moduleHandler()->moduleExists('comment') && !empty($this->options['comments']) ?  $this->getValue($values, 'last_comment') : 0;
+++ b/core/modules/history/lib/Drupal/history/Plugin/views/filter/HistoryUserTimestamp.phpundefined
@@ -75,7 +75,7 @@ public function query() {
-    if (module_exists('comment')) {
+    if (\Drupal::moduleHandler()->moduleExists('comment')) {
+++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/Node.phpundefined
@@ -33,7 +33,7 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
-      if (module_exists('translation')) {
+      if (\Drupal::moduleHandler()->moduleExists('translation')) {
+++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/Revision.phpundefined
@@ -31,7 +31,7 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
-      if (module_exists('translation')) {
+      if (\Drupal::moduleHandler()->moduleExists('translation')) {
@@ -66,7 +66,7 @@ function render_link($data, ResultRow $values) {
-      if (module_exists('translation')) {
+      if (\Drupal::moduleHandler()->moduleExists('translation')) {

This can be injected with by creating a static create method on the class... see Drupal\views\Plugin\views\style\Table

Hi Alex,

Thanks for the review. I will take a look.

Sam

How we going with this?

Status:Needs work» Needs review
StatusFileSize
new26.33 KB
PASSED: [[SimpleTest]]: [MySQL] 58,258 pass(es).
[ View ]

I have attached the patch for the procedural code only. The OO code can be tackled in a later patch.

Status:Needs review» Reviewed & tested by the community

Looks good.

Status:Reviewed & tested by the community» Needs work

no longer applies.

Assigned:Sam152» Unassigned
Status:Needs work» Needs review
StatusFileSize
new21.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,636 pass(es).
[ View ]

I tried to re-rolling.

The patch in #24 applied cleanly for me and everything looks OK. (Although I had to delete sites/default/files/php to get up and running again.)

As per #25 - patch applied cleanly, but had to empty sites/default/files/php/ to get back up and running.

We've now standardised on "\Drupal" in core regardless of whether it's in a procedural or OO file so this patch needs updating to match that.

Issue tags:-Novice+clean system cleanup
StatusFileSize
new21.7 KB
new20.78 KB
PASSED: [[SimpleTest]]: [MySQL] 59,095 pass(es).
[ View ]
new20.75 KB
PASSED: [[SimpleTest]]: [MySQL] 59,085 pass(es).
[ View ]

Re-roll as the comment module was failing to apply. And fix from #27

#28 looks good and applies cleanly.

Status:Needs review» Reviewed & tested by the community

#28 applied cleanly and looks fine.

Looks good.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new20.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,549 pass(es).
[ View ]

Reroll of #28 (the 20.78kb one) as the first hunk in node.views.inc has been removed already in #2057401: Make the node entity database schema sensible

Otherwise it is the same.

Status:Needs review» Needs work
Issue tags:-clean system cleanup

The last submitted patch, 2045931-32.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+clean system cleanup

#32: 2045931-32.patch queued for re-testing.

StatusFileSize
new19.28 KB
PASSED: [[SimpleTest]]: [MySQL] 58,624 pass(es).
[ View ]

And another reroll as comment.module and comment.views.inc are already done in #2003498: Replace drupal_container() with Drupal::service() in the comment module

Status:Needs review» Reviewed & tested by the community

Back to RTBC

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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

Issue summary:View changes
Status:Closed (fixed)» Active

Reopening as there are still loads of calls to module_exists().

Status:Active» Needs review
StatusFileSize
new18.63 KB
PASSED: [[SimpleTest]]: [MySQL] 63,089 pass(es).
[ View ]

Ok here is all but the function itself. Should that be included as well or marked as deprecated?

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/DBLogResource.php
@@ -27,7 +27,7 @@ class DBLogResource extends ResourceBase {
+    if (\Drupal::moduleHandler()->moduleExists('dblog')) {
...
-    if (module_exists('dblog')) {

This is in OO code, so shouldn't use \Drupal. There are several examples of this in the patch.

Ok here is all but the function itself. Should that be included as well or marked as deprecated?

Let's leave it for now, and then remove all the module_* functions together for the meta. It would be good to expand the comment though, to make it clear that it's not going to be around much longer. Suggested comment as discussed on #2158871: [Policy, no patch] Clearly denote when @deprecated code is slated to be removed would be

/**
* Determines whether a module implements a hook.
*
* @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0-beta1, see https://drupal.org/node/1916134
*
* @see https://drupal.org/node/1894902
*
* @see \Drupal\Core\Extension\ModuleHandler::implementsHook()
*/

Status:Needs review» Needs work
Issue tags:+@deprecated

Regarding using \Drupal in OO code, the same problem applies in #2055371: Replace all module_invoke_all() deprecated function calls in OO code, where it has been decided to use \Drupal for now, then do propery dependency injection in a later patch. We can do the same here DI is going to be tricky, but at the very least CustomBlockTypeFormController has a $moduleHandler property that we can use (I've not checked the others).

Status:Needs work» Needs review
StatusFileSize
new1.74 KB
new18.62 KB
PASSED: [[SimpleTest]]: [MySQL] 63,336 pass(es).
[ View ]

Replaced two \Drupal::moduleHandler() calls in CustomBlockTypeFormController and ConfigTestFormController as mentioned in #44.

Status:Needs review» Needs work

moving back to needs work, based on @ianthomas_uk's suggestion in #41 to either remove the deprecated function, or expand the comments.

Status:Needs work» Reviewed & tested by the community

Actually, the functions for this meta are all next to each other, so if we remove or edit the docblock on one then it will trigger a reroll for one of the other patches.

Let's just remove the uses, then we can follow up with a quick patch to remove the functions themselves.

We still need a followup to switch to DI.

Status:Reviewed & tested by the community» Fixed

Yes removing uses then separate patch to remove the actual functions is the easiest way to do these - reduces the chance of broken HEAD due to commit conflicts. And the removal patches themselves are much easier to re-roll/roll-back in case of conflicts.

Status:Fixed» Closed (fixed)

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