Move global constants from *.module files into interfaces

@Wim Leers
There are multiple interfaces defined for each module not sure where to define the constants removed from the .module files.
Can you please help to let us know the same ?

@Prashant.c: you look up a fitting interface in the module that defines the constant and put it there. Make sure to remove the module name prefix of the constant in the process.

Example: AGGREGATOR_CLEAR_NEVER should go to Drupal\aggregator\FeedInterface I think.

@Wim Leers

Thanks for the list, this will surely help a lot.

Let's please take the approach from #2807961: Replace usages of deprecated global constants with the new interface constants which does the actual usage moving as a script change, rather than a manual hand crafted one.

Filed #2808065: Add a sniff to disallow global constants in Drupal 8 in Coder.

I'll be working on this at Drupalcon Dublin 2016

Added a patch covering comment #6. Working in the rest ones, waiting for confirmation in @DrupalconDubling2016

Added a patch covering comment #6. Working in the rest ones, waiting for confirmation in @DrupalconDubling2016

Make sure to look at the commit in #2807263: Impossible to write unit tests involving Vocabulary, because TAXONOMY_HIERARCHY_(DISABLED|SINGLE|MULTIPLE) are defined in taxonomy.module instead of VocabularyInterface how we deprecate the constants, because it seems you are removing them?

1. +++ b/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -147,7 +147,7 @@ class DefaultMenuLinkTreeManipulators {
            $access_result->addCacheContexts(['user.node_grants:view']);
   -        $query->condition('status', NODE_PUBLISHED);
   +        $query->condition('status', PUBLISHED);
   

   this change seems wrong. There should be a class reference with the new constant. Also elsewhere.

2. +++ b/modules/aggregator/aggregator.module
   @@ -8,10 +8,6 @@
   -/**
   - * Denotes that a feed's items should never expire.
   - */
   -const AGGREGATOR_CLEAR_NEVER = 0;
   

   we cannot remove constants, we can only deprecate them

awesome, thanks klausi. I'll be using that approach :) (I'm new to D8)

just to make sure you are talking to change this, right?

from
$hierarchy = TAXONOMY_HIERARCHY_DISABLED;

to
$hierarchy = VocabularyInterface::HIERARCHY_DISABLED;

If so I'll get it done this week :D

I'm wondering whether we really want to both move the constants and replace the usages at the same time. At least the second bit could be scripted and by that less erorrs might be made.

Hi there !

Here is a patch inwhich I move all constants in Interface (using Wim Leers recommandations, comment #6)
I declare constants in Interface and use theses to init constants in .module files, to be compliant with other files and contrib modules.

Also, as there were no recommandations for 6 constants, I moved theses in UserInterface (DRUPAL_USER_TIMEZONE_DEFAULT, DRUPAL_USER_TIMEZONE_EMPTY, DRUPAL_USER_TIMEZONE_SELECT) and FormInterface (DRUPAL_DISABLED, DRUPAL_OPTIONAL, DRUPAL_REQUIRED).

Please have a look and tell me what you think about it.

Same as in #2807263-29: Impossible to write unit tests involving Vocabulary, because TAXONOMY_HIERARCHY_(DISABLED|SINGLE|MULTIPLE) are defined in taxonomy.module instead of VocabularyInterface:

PHP Fatal error:  Class 'Drupal\user\UserInterface' not found in /var/www/html/core/modules/system/system.module on line 39

We cannot use the class in the global scope because the class loader for the module is not there when the module is installed/uninstalled.

You need to leave the hard coded values there unmodified.

Some minor nitpicks:

1. +++ b/core/lib/Drupal/Core/Form/FormInterface.php
   @@ -10,6 +10,21 @@
      /**
   +   * Disabled option on forms and settings
   +   */
   +  const DRUPAL_DISABLED = 0;
   

   The DRUPAL prefix here is redundant and should be removed.

2. +++ b/core/modules/aggregator/aggregator.module
   @@ -6,12 +6,16 @@
   + *
   + * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
   + * Use \Drupal\aggregator\FeedStorageInterface::CLEAR_NEVER instead.
   

   deprecation message is wrong. We did not deprecate this in 8.0.0. Simply use "Scheduled for removal in Drupal 9.0.0. Use .... instead."

Thanks Klausi. I thoulght it was some thing llike that.

But how can I move constants into interfaces if I cannot load them on site installation ?!

You just copy them to interfaces. During site installation the classes should be there, only if a single module is installed the classes of that module might not be ready when the module file is included.

You mean I do not declare constants in module files using interface.
It means double declaration ?
That may be confusing !?

Exactly, it is not ideal, but unfortunately we can only deprecate them like that.

Here is a new patch inwhich I double declare some constants, to make site installation successfull.
I also modify deprecation warning.

1. +++ b/core/modules/aggregator/aggregator.module
   @@ -6,12 +6,16 @@
    /**
     * Denotes that a feed's items should never expire.
   + *
   + * Scheduled for removal in Drupal 9.0.0.
   + * Use \Drupal\aggregator\FeedStorageInterface::CLEAR_NEVER instead.
     */
   

   This should have an @deprecated tag with the sentence after it. Please check all deprecations and add the @deprecated tag back.

2. +++ b/core/modules/aggregator/aggregator.module
   @@ -6,12 +6,16 @@
   -const AGGREGATOR_CLEAR_NEVER = 0;
   +const AGGREGATOR_CLEAR_NEVER = FeedStorageInterface::CLEAR_NEVER;
   

   As I said, the constant should keep the hard coded value here. Please check all constants in all module files.

+++ b/core/lib/Drupal/Core/Datetime/DateFormatterInterface.php
@@ -8,6 +8,21 @@
 interface DateFormatterInterface {
 
   /**
+   * Defines the timezone that dates should be stored in.
+   */
+  const STORAGE_TIMEZONE = 'UTC';
+
+  /**
+   * Defines the format that date and time should be stored in.
+   */
+  const DATETIME_STORAGE_FORMAT = 'Y-m-d\TH:i:s';
+
+  /**
+   * Defines the format that dates should be stored in.
+   */
+  const DATE_STORAGE_FORMAT = 'Y-m-d';
+

These have nothing to do with the dateformatter service; they relate to datetime field storage. We don't have an interface for DateTimeItem. I suggest removing this hunk (and related changes), and we can handle in #2826404: Create DateTimeItemInterface and deprecate global constants in datetime.module.

OK.
Here is a new version of the patch, inwhich I double-declare listed constants in .module files and interfaces.

Sorry, forgot the «Needs review» status..

1. +++ b/core/lib/Drupal/Core/Datetime/DateFormatterInterface.php
   @@ -8,6 +8,21 @@
   +   * Defines the timezone that dates should be stored in.
   +   */
   +  const STORAGE_TIMEZONE = 'UTC';
   +
   +  /**
   +   * Defines the format that date and time should be stored in.
   +   */
   +  const DATETIME_STORAGE_FORMAT = 'Y-m-d\TH:i:s';
   +
   +  /**
   +   * Defines the format that dates should be stored in.
   +   */
   +  const DATE_STORAGE_FORMAT = 'Y-m-d';
   +
   +  /**
       * Formats a date, using a date type or a custom date format string.
   

   This is wrong. This is the general interface for date formaters; the constants are about the datetime fields.

2. +++ b/core/modules/datetime/datetime.module
   @@ -9,16 +9,27 @@
    /**
     * Defines the timezone that dates should be stored in.
   + *
   + * Scheduled for removal in Drupal 9.0.0.
   + * Use \Drupal\Core\Datetime\DateFormatterInterface::STORAGE_TIMEZONE instead.
     */
    const DATETIME_STORAGE_TIMEZONE = 'UTC';
    
    /**
     * Defines the format that date and time should be stored in.
   + *
   + * Scheduled for removal in Drupal 9.0.0.
   + * Use \Drupal\Core\Datetime\DateFormatterInterface::DATETIME_STORAGE_FORMAT
   + * instead.
     */
    const DATETIME_DATETIME_STORAGE_FORMAT = 'Y-m-d\TH:i:s';
    
    /**
     * Defines the format that dates should be stored in.
   + *
   + * Scheduled for removal in Drupal 9.0.0.
   + * Use \Drupal\Core\Datetime\DateFormatterInterface::DATE_STORAGE_FORMAT
   + * instead.
     */
    const DATETIME_DATE_STORAGE_FORMAT = 'Y-m-d';
    
   

I would really prefer to handle these in #2826404: Create DateTimeItemInterface and deprecate global constants in datetime.module.

Hello mpdonadio !
Didn't read your comment about Date constants, sorry !

So, I'm supposed to remove modifications in datetime.module and DateFormatterInterface ?
And let you deprecated theses 3 constants and declare them in new interfaces?

So, I'm supposed to remove modifications in datetime.module and DateFormatterInterface? And let you deprecated theses 3 constants and declare them in new interfaces?

Yeah, just back out those changes and we will handle in in #2826404: Create DateTimeItemInterface and deprecate global constants in datetime.module.

Also:

+++ b/core/modules/aggregator/aggregator.module
@@ -10,6 +10,9 @@
+ *
+ * Scheduled for removal in Drupal 9.0.0.
+ * Use \Drupal\aggregator\FeedStorageInterface::CLEAR_NEVER instead.

@deprecated tag is missing in the beginning. Please check all deprecations that they have the tag.

@mpdonadio.
Thx for all, I will provide a new version as soon as possible, without DateTime constants modifications.

@klausi.
OK, but since which version theses constants are deprecated ?
I mentionned that in a previous patch, and you tell me to remove in comment #23.
Should the new message be :

/*
 * @deprecated in Drupal 8.3.0, will be removed before Drupal 9.0.0.
 *   Use ..... instead.
 */

#38, this pattern is used several times in core:

/**
  * Defines the format that dates should be stored in.
+ *
+ * @deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use
+ *   \Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface::DATE_STORAGE_FORMAT
+ *   instead.
  */

Note the two-space indent on the second and third lines.

OK there are new version for patch, without DateTime modifications, and with a new syntax to deprecate constants. Please have a look !

Thx to @mpdonadio and @klausi.

Some updates to the issue here and a review.

It looks like the patch in #40 deprecates all *.module constants excluding those which were already deprecated, and datetime and locale modules.

Datetime has a follow-up issue about this #2826404: Create DateTimeItemInterface and deprecate global constants in datetime.module. I don't see one for locale.

The patch does not perform usage replacements, which will happen in #2807961: Replace usages of deprecated global constants with the new interface constants

I'd RTBC but we need a follow-up for locale as per #6.

Hey Mile23 !
I could create a dedicated issue for Locale Interface, and move all constants into this new interface.

Current issue could be RTBC if so ?

Created #2831617: Deprecate global constants in locale module. Still need CR before we can RTBC this.

Rough pass at a CR made.

Patch no longer applies. CRs and followups are set, so we just need the reroll part.

Just a simple rebase was needed.

$ git checkout ee23c18
$ git apply --index no-constant-in-modules-2807785-40.patch
$ git rebase origin/8.3.x
First, rewinding head to replay your work on top of it...
Applying: no-constant-in-modules-2807785-40.patch
Using index info to reconstruct a base tree...
M	core/modules/comment/src/CommentInterface.php
M	core/modules/node/src/NodeInterface.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/node/src/NodeInterface.php
Auto-merging core/modules/comment/src/CommentInterface.php

There's an older similar issue but only for nodes which replaces also all the occurrences of the constants: #2313095: Move node constants to NodeInterface.

We cannot use the class in the global scope because the class loader for the module is not there when the module is installed/uninstalled.

You need to leave the hard coded values there unmodified.

@klausi, This is revealing a bug in ModuleInstaller. The problem is that, on install, the module files "$module.module" and "$module.install" are loaded before the module's PSR4 namespaces are registered. In this case, the code that is executed on code file load (like constant declarations) cannot benefit from the new namespaces. I'm adding also the ModuleInstaller fix and with this we can avoid hard-coding of values at least for constants declared in interfaces belonging to same module and core. I changed the deprecated constant declaration only for those cases.

• Rerolled because #2313095: Move node constants to NodeInterface went in.
• Fixing the IS
• Reverted changes from #49.

EDIT: Regarding the CR, should we extend (and rename) https://www.drupal.org/node/2316145 or we create a new one? In the meantime I saw that we always have a CR at https://www.drupal.org/node/2831620

Simplified the title.

Postponed #2807961: Replace usages of deprecated global constants with the new interface constants on this.

It all looks good to me, but I have some questions:

1. How about datetime.module?

   /**
    * Defines the timezone that dates should be stored in.
    */
   const DATETIME_STORAGE_TIMEZONE = 'UTC';
   
   /**
    * Defines the format that date and time should be stored in.
    */
   const DATETIME_DATETIME_STORAGE_FORMAT = 'Y-m-d\TH:i:s';
   
   /**
    * Defines the format that dates should be stored in.
    */
   const DATETIME_DATE_STORAGE_FORMAT = 'Y-m-d';
   

2. How about the locale.module?
3. How about the locale.translocation.inc?
4. All deprecations refere to 8.3 and the issue is now for 8.4. Can it still get into 8.3 or will this be a 8.4 commit?

@daffie, For 1, 2, 3 please read all the comments ^^^

All deprecations refere to 8.3 and the issue is now for 8.4. Can it still get into 8.3 or will this be a 8.4 commit?

This is not a bug, so will go in 8.4.x (which is the development branch). But it can be backported in 8.3.x.

Moving the issue back to 8.3.

It all looks good to me.
For me it is RTBC.

Great @daffie. About the version the committers will decide.

Updated also the CR to reflect the changes: https://www.drupal.org/node/2831620

Let's not invent a new @deprecated line standard. This should be

@deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.0. 

instead of "Drupal 9.0.x"

@klausi, good catch

Voilà!

All 9.0.x instances are changed to 9.0.0.
Back to RTBC.

1. +++ b/core/lib/Drupal/Core/Form/FormInterface.php
   @@ -10,6 +10,21 @@
   +   * Disabled option on forms and settings
   +   */
   +  const DISABLED = 0;
   +
   +  /**
   +   * Optional option on forms and settings
   +   */
   +  const OPTIONAL = 1;
   +
   +  /**
   +   * Required option on forms and settings
   +   */
   +  const REQUIRED = 2;
   +
   

   Those DRUPAL_* constants are super weird.

   I'm not sure if we should just replace them 1:1 in the form component.

   I can only see 3 uses of them, node preview, comment preview and link title. Maybe each should define its own constants?

2. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManagerInterface.php
   @@ -23,6 +23,12 @@
   +   * Maximum length of menu name as entered by the user. Database length is 32
   +   * and we add a menu- prefix.
   

   do we really, I don't think so?

3. +++ b/core/modules/responsive_image/src/ResponsiveImageStyleInterface.php
   @@ -10,6 +10,12 @@
      /**
   +   * The machine name for the empty image breakpoint image style option.
   +   */
   +  const EMPTY_IMAGE = '_empty image_';
   +  const ORIGINAL_IMAGE = '_original image_';
   +
   +  /**
   

   I don't think we can not have docs for the second just because the old constants didn't have either? new code needs to follow coding standards.

#64-2, that looks like the old D7 comment, where that was true (and IIRC, still has a nasty bug when resaving). Drupal 8 `menu_name` fields don't get the 'menu-' prefix in storage.

So the question is, do we even still use that constant? I kinda doubt it. And if not, then we could just deprecate it without replacement.

#66, attached is a usage report from PhpStorm. It looks accurate.

\Drupal\Core\Menu\MenuTreeStorage::schemaDefinition() hardcodes the 'menu_name' column to 32 using the literal.

The `MAX_MENU_NAME_LENGTH_UI ` constant does look like it is used just for the UI form element, and then in test coverage. As far as I can tell, the comment is wrong (as well as the value...). Prob want @pwolanin or @dawehner to weigh in, though; I haven't been in menu code in a while.

If the scope of the issue is

This issue is only creating the constants in interfaces and is deprecating the old constants

then we probably want to leave this and potentially file a followups for wrong docs, dead code, etc

?

@mpdonadio: totally agreed, this issue is just about moving stuff, not fixing all the things that have ever been wrong with our constants. We can do all of that in follow-ups, please file them.

Not going to move back again but I disagree with that. I don't see the point in moving constants around and then in in follow-ups deprecate those another time. If anything, then we should skip those that need more discussion.

We have to live with deprecated code for quite a while and there is really no reason to add code that we know we're going to deprecate again.

I don't see any argument why this would be important enough to live with that, constants in modules are not nice and annoying in unit tests, but we've been working around that for years.

Thinking more on #64...

#64.3: This totally makes sense, at least because with patch from #61, RESPONSIVE_IMAGE_ORIGINAL_IMAGE is not marked as deprecated in the IDE lacking its own @deprecated tag.

#64.2: That is a config entity ID, having its own limit.

#64.1: Created #2851705: Each class using DRUPAL_* constants from system.module should define its own constants.

So #70 is NR then. Thanks!

Thanks, This looks good to me now.

Do we already have issues to get rid of the deprecated usages? Wondering if we want to do (some) replacements already here.

I guess the committers can decide that.

And last, not convinced this is actually major, this doesn't actually block anything specifically as far as I ee.

I think I'd prefer to do the replacements in followup issues. Thanks @Berdir!

I really like this patch. I agree it is not major though.

1. +++ b/core/modules/menu_ui/menu_ui.module
   @@ -22,8 +22,11 @@
   + * Maximum length of menu name as entered by the user.
   + *
   + * @deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.0. Use
   + *   \Drupal\Core\Config\Entity\ConfigEntityStorage::MAX_ID_LENGTH because the
   + *   menu name is a configuration entity ID.
     */
    const MENU_MAX_MENU_NAME_LENGTH_UI = 27;
   

   Bonus. Hm. Is this constant my fault?

2. +++ b/core/modules/update/update.module
   @@ -16,50 +16,79 @@
   -// These are internally used constants for this code, do not modify.
   +/*
   + * These are internally used constants for this code, do not modify.
   + */
   

   Oh-so-nitpick: Missing second star on the first line of the new docblock.

   But actually, this fix is out of scope anyway.

3. +++ b/core/modules/user/src/UserInterface.php
   @@ -14,6 +14,44 @@
   +  /**
   +   * Visitors can create accounts, but they don't become active without
   +   * administrative approval.
   +   */
   

   This one-line summary is two lines.

4. I am not quite grokking #2851705: Each class using DRUPAL_* constants from system.module should define its own constants; left a comment there.
5. A couple things in the CR https://www.drupal.org/node/2831620 look like they need updating for the latest patch.
6. Regarding this point in the summary:
   

   Also add a PHPCS rule to automatically detect this in contrib modules and warn/prevent this (issue to be created).

   I think this is covered by the overall plan to use the phpunit bridge to test for deprecated code? #2488860: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation Unless it means "complain at people for using global constants"?

I reviewed all the added constants and deprecation to check that they lined up. Those first couple bugs are things I can probably fix on commit, so leaving RTBC for the moment.

I figured out #74.6. Those three constants were removed from the patch, because the HEAD versions are weirdly broad and putting them on FormInterface is also weird, and the last thing we want to do is add more constants we need to provide BC for in the wrong place. I'll update the summary of the other issue.

I updated the CR. #74.1 might just mean that one constant needs a slightly different followup. Tagging "Needs followup" for #6 in case someone wants it to be a global coder rule about not having global constants, but for me that isn't really a requirement, and as mentioned if people want coder checks for @deprecated that's coming in even better form (hopefully) with automated testing. I hope. Some day.

I'll fix the other two things on commit.

BTW this should have been left filed against 8.4.x, not 8.3.x, since it includes API additions. See https://groups.drupal.org/node/515932 and https://www.drupal.org/core/d8-allowed-changes#alpha. However, @catch and I agreed to backport this to 8.3.x so long as it was ready before the beta. For other minor target issues though please don't change the branch back to 8.3.x; the changes are only backported after commit at committer discretion during the alpha and the alpha is over in like 36h. :)

Fixes on commit:

</code>diff --git a/core/modules/update/update.module b/core/modules/update/update.module
index 30c01b70aa..6ef4628210 100644
--- a/core/modules/update/update.module
+++ b/core/modules/update/update.module
@@ -16,9 +16,7 @@
 use Drupal\Core\Routing\RouteMatchInterface;
 use Drupal\Core\Site\Settings;
 
-/*
- * These are internally used constants for this code, do not modify.
- */
+// These are internally used constants for this code, do not modify.
 
 /**
  * Project is missing security update(s).
diff --git a/core/modules/user/src/UserInterface.php b/core/modules/user/src/UserInterface.php
index 291a149a73..6919e0d7b4 100644
--- a/core/modules/user/src/UserInterface.php
+++ b/core/modules/user/src/UserInterface.php
@@ -31,8 +31,7 @@
   const REGISTER_VISITORS = 'visitors';
 
   /**
-   * Visitors can create accounts, but they don't become active without
-   * administrative approval.
+   * Visitors can create accounts that only become active with admin approval.
    */
   const REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL = 'visitors_admin_approval';
 
diff --git a/core/modules/user/user.module b/core/modules/user/user.module
index 00205a6249..638ed649e5 100644
--- a/core/modules/user/user.module
+++ b/core/modules/user/user.module
@@ -56,7 +56,7 @@
  *
  * @deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.0.
  *   Use \Drupal\user\UserInterface::REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL
- * instead.
+ *   instead.
  */
 const USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL = 'visitors_admin_approval';
 

Instead of a PHPCS followup, I wonder if we should have a coding standards project not to use global constants? The PHPCS rule would follow if we adopted that standard across Drupal.

Anyway, committed to 8.4.x and 8.3.x. Thanks everyone!