Support from Acquia helps fund testing for Drupal Acquia logo

Comments

benjy’s picture

Issue tags: +Novice

Tagging

Sam152’s picture

Assigned: Unassigned » Sam152

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

Sam152’s picture

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.

benjy’s picture

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.

Sam152’s picture

Status: Needs review » Active
FileSize
57.68 KB

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

Sam152’s picture

Status: Active » Needs review
Sam152’s picture

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.

Sam152’s picture

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

Sam152’s picture

Status: Needs work » Needs review
benjy’s picture

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.

Sam152’s picture

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

benjy’s picture

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

Sam152’s picture

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.

Sam152’s picture

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

Sam152’s picture

Status: Needs work » Needs review
benjy’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

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

Sam152’s picture

Hi Alex,

Thanks for the review. I will take a look.

Sam

benjy’s picture

How we going with this?

Sam152’s picture

Status: Needs work » Needs review
FileSize
26.33 KB

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

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

no longer applies.

adsw12’s picture

Assigned: Sam152 » Unassigned
Status: Needs work » Needs review
FileSize
21.7 KB

I tried to re-rolling.

johnmcc’s picture

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

galooph’s picture

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

benjy’s picture

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.

joelpittet’s picture

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

johnmcc’s picture

#28 looks good and applies cleanly.

galooph’s picture

Status: Needs review » Reviewed & tested by the community

#28 applied cleanly and looks fine.

benjy’s picture

Looks good.

LinL’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
20.5 KB

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.

LinL’s picture

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

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

LinL’s picture

FileSize
19.28 KB

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

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

ianthomas_uk’s picture

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

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

joelpittet’s picture

Status: Active » Needs review
FileSize
18.63 KB

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

ianthomas_uk’s picture

+++ 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()
 */
ianthomas_uk’s picture

Status: Needs review » Needs work
Issue tags: +@deprecated
ianthomas_uk’s picture

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

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
1.74 KB
18.62 KB

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

herom’s picture

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.

ianthomas_uk’s picture

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.

catch’s picture

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.