Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Global constants in .module files are creating problems:
- Makes hard unit testing. #2807263: Impossible to write unit tests involving Vocabulary, because TAXONOMY_HIERARCHY_(DISABLED|SINGLE|MULTIPLE) are defined in taxonomy.module instead of VocabularyInterface uncovered the case of
TAXONOMY_HIERARCHY_(DISABLED|SINGLE|MULTIPLE)
- Page cache serializes content when modules are loaded.
Note that, for good reasons, there are such conversions handled in other issues,:
- #2313095: Move node constants to NodeInterface
- #2826404: Create DateTimeItemInterface and deprecate global constants in datetime.module
- #2831617: Deprecate global constants in locale module
Proposed resolution
Move all of these to interfaces, and deprecate the existing constants. This retains BC. This issue is only creating the constants in interfaces and is deprecating the old constants, the replacement of usages will be done by using a script, in #2807961: Replace usages of deprecated global constants with the new interface constants.
- AGGREGATOR_CLEAR_NEVER → \Drupal\aggregator\FeedStorageInterface
- COMMENT_ANONYMOUS_* →\Drupal\comment\CommentInterface
- MENU_MAX_MENU_NAME_LENGTH_UI →\Drupal\Core\Menu\MenuLinkManagerInterface
- RESPONSIVE_IMAGE_* →\Drupal\responsive_image\ResponsiveImageStyleInterface
- REGIONS_* →\Drupal\block\BlockRepositoryInterface
- positive UPDATE_* →\Drupal\update\UpdateManagerInterface
- negative UPDATE_* →\Drupal\update\UpdateFetcherInterface
- USER* →\Drupal\user\UserInterface
Remaining tasks
- Replace all usages using a script in #2807961: Replace usages of deprecated global constants with the new interface constants
- Also add a PHPCS rule to automatically detect this in contrib modules and warn/prevent this (issue to be created).
User interface changes
None.
API changes
See CR: https://www.drupal.org/node/2831620
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#70 | interdiff-61-to-70.txt | 4.76 KB | claudiu.cristea |
#70 | 2807785-70.patch | 14.39 KB | claudiu.cristea |
Comments
Comment #2
Wim LeersComment #3
sneha_surve CreditAttribution: sneha_surve at Iksula commentedComment #4
Prashant.c@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 ?
Comment #5
klausi@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.
Comment #6
Wim LeersHere you go:
AGGREGATOR_CLEAR_NEVER
→\Drupal\aggregator\FeedStorageInterface
COMMENT_ANONYMOUS_*
→\Drupal\comment\CommentInterface
DATETIME_*
→\Drupal\Core\Datetime\DateFormatterInterface
FILE_URL_TEST_CDN_*
can be ignored, because this is a test moduleIMAGE_STORAGE_NORMAL
can be ignored, because they're already deprecatedLOCALE_*
would be better ignored for now I think, there's still a lot of procedural code in thereMENU_MAX_MENU_NAME_LENGTH_UI
→\Drupal\Core\Menu\MenuLinkManagerInterface
NODE_*
→\Drupal\node\NodeInterface
RESPONSIVE_IMAGE_*
→\Drupal\responsive_image\ResponsiveImageStyleInterface
REGIONS_*
→\Drupal\block\BlockRepositoryInterface
ENTITY_TEST_TYPES_*
can be ignored, because this is a test moduleUPDATE_*
→\Drupal\update\UpdateManagerInterface
UPDATE_*
→\Drupal\update\UpdateFetcherInterface
USER*
→\Drupal\user\UserInterface
This would solve 99% of the problem.
Comment #7
Prashant.c@Wim Leers
Thanks for the list, this will surely help a lot.
Comment #8
dawehnerLet'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.
Comment #9
klausiFiled #2808065: Add a sniff to disallow global constants in Drupal 8 in Coder.
Comment #10
cllamas CreditAttribution: cllamas as a volunteer and at Dennis commentedI'll be working on this at Drupalcon Dublin 2016
Comment #11
klausiComment #12
cllamas CreditAttribution: cllamas as a volunteer and at Dennis commentedAdded a patch covering comment #6. Working in the rest ones, waiting for confirmation in @DrupalconDubling2016
Comment #13
cllamas CreditAttribution: cllamas as a volunteer and at Dennis commentedAdded a patch covering comment #6. Working in the rest ones, waiting for confirmation in @DrupalconDubling2016
Comment #14
klausiMake 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?
this change seems wrong. There should be a class reference with the new constant. Also elsewhere.
we cannot remove constants, we can only deprecate them
Comment #15
cllamas CreditAttribution: cllamas as a volunteer and at Dennis commentedawesome, 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
Comment #16
dawehnerI'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.
Comment #17
OwilliwOHi 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.
Comment #18
OwilliwOComment #22
klausiSame 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:
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.
Comment #23
klausiSome minor nitpicks:
The DRUPAL prefix here is redundant and should be removed.
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."
Comment #24
OwilliwOThanks 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 ?!
Comment #25
klausiYou 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.
Comment #26
OwilliwOYou mean I do not declare constants in module files using interface.
It means double declaration ?
That may be confusing !?
Comment #27
klausiExactly, it is not ideal, but unfortunately we can only deprecate them like that.
Comment #28
OwilliwOHere is a new patch inwhich I double declare some constants, to make site installation successfull.
I also modify deprecation warning.
Comment #30
klausiThis should have an @deprecated tag with the sentence after it. Please check all deprecations and add the @deprecated tag back.
As I said, the constant should keep the hard coded value here. Please check all constants in all module files.
Comment #31
mpdonadioThese 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.
Comment #32
OwilliwOOK.
Here is a new version of the patch, inwhich I double-declare listed constants in .module files and interfaces.
Comment #33
OwilliwOSorry, forgot the «Needs review» status..
Comment #34
mpdonadioThis is wrong. This is the general interface for date formaters; the constants are about the datetime fields.
I would really prefer to handle these in #2826404: Create DateTimeItemInterface and deprecate global constants in datetime.module.
Comment #35
OwilliwOHello 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?
Comment #36
mpdonadioYeah, just back out those changes and we will handle in in #2826404: Create DateTimeItemInterface and deprecate global constants in datetime.module.
Comment #37
klausiAlso:
@deprecated tag is missing in the beginning. Please check all deprecations that they have the tag.
Comment #38
OwilliwO@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 :
Comment #39
mpdonadio#38, this pattern is used several times in core:
Note the two-space indent on the second and third lines.
Comment #40
OwilliwOOK 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.
Comment #41
Mile23Some 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.
Comment #42
Mile23Comment #43
OwilliwOHey 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 ?
Comment #44
mpdonadioCreated #2831617: Deprecate global constants in locale module. Still need CR before we can RTBC this.
Comment #45
mpdonadioRough pass at a CR made.
Comment #46
Mile23Comment #47
Mile23Patch no longer applies. CRs and followups are set, so we just need the reroll part.
Comment #48
mpdonadioJust a simple rebase was needed.
Comment #49
claudiu.cristeaThere's an older similar issue but only for nodes which replaces also all the occurrences of the constants: #2313095: Move node constants to NodeInterface.
@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.
Comment #51
claudiu.cristeaEDIT:
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/2831620Comment #52
claudiu.cristeaSimplified the title.
Comment #53
claudiu.cristeaPostponed #2807961: Replace usages of deprecated global constants with the new interface constants on this.
Comment #54
daffie CreditAttribution: daffie commentedIt all looks good to me, but I have some questions:
Comment #55
claudiu.cristea@daffie, For 1, 2, 3 please read all the comments ^^^
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.
Comment #56
daffie CreditAttribution: daffie commentedMoving the issue back to 8.3.
It all looks good to me.
For me it is RTBC.
Comment #57
claudiu.cristeaGreat @daffie. About the version the committers will decide.
Comment #58
claudiu.cristeaUpdated also the CR to reflect the changes: https://www.drupal.org/node/2831620
Comment #59
klausiLet's not invent a new @deprecated line standard. This should be
instead of "Drupal 9.0.x"
Comment #60
claudiu.cristea@klausi, good catch
Comment #61
claudiu.cristeaVoilà!
Comment #62
cilefen CreditAttribution: cilefen commentedComment #63
daffie CreditAttribution: daffie commentedAll 9.0.x instances are changed to 9.0.0.
Back to RTBC.
Comment #64
BerdirThose 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?
do we really, I don't think so?
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.
Comment #65
mpdonadio#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.
Comment #66
BerdirSo 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.
Comment #67
mpdonadio#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
then we probably want to leave this and potentially file a followups for wrong docs, dead code, etc
?
Comment #68
klausi@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.
Comment #69
BerdirNot 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.
Comment #70
claudiu.cristeaThinking 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.
Comment #71
xjmSo #70 is NR then. Thanks!
Comment #72
BerdirThanks, 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.
Comment #73
xjmI think I'd prefer to do the replacements in followup issues. Thanks @Berdir!
Comment #74
xjmI really like this patch. I agree it is not major though.
Bonus. Hm. Is this constant my fault?
Oh-so-nitpick: Missing second star on the first line of the new docblock.
But actually, this fix is out of scope anyway.
This one-line summary is two lines.
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.
Comment #75
xjmI 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.
Comment #76
xjmI 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.
Comment #77
xjmBTW 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. :)
Comment #81
xjmFixes on commit:
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!