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
Constants in bootstrap.inc are difficult to test and require weird hacks in our unit tests. When the constant is on a Class or interface they can be autoloaded and provide additional context about their purpose and meaning.
UserTest documents that this should be done as well but there is no issue attached to the @todo.
Proposed resolution
Move them to RoleInterface?
Remaining tasks
Provide a Patch
API changes
2 Constants are being renamed placed on and Interface instead of existing in bootstrap.inc
Beta phase evaluation
Issue category | Task. This is part of our modernization and not providing a new feature or fixing any bugs |
---|---|
Issue priority | Not critical. Have significant usability gains or community consensus that I know of. |
Unfrozen changes | ? |
Disruption | Could be disruptive for core/contrib because it changes constants that get a fair amount of us. It could be mitigated by providing BC in the form of a deprecated global constant. |
Comment | File | Size | Author |
---|---|---|---|
#26 | move_role_constants_on-2433281-26..interdiff.txt | 4.1 KB | neclimdul |
#26 | move_role_constants_on-2433281-26.patch | 97.8 KB | neclimdul |
Comments
Comment #1
neclimdulI'd like to say this is unfrozen because there was an existing @todo but it was hidden in a test and not clearly documented around the constants that feels a bit disingenuous.
Comment #2
neclimdulA possible implementation.
Comment #3
dawehnerMHHHH, I think its wrong to add a dependency of user module onto all those code.
Comment #4
dawehnerMabye
\Drupal
orAccountInterface
is a better place for it?Comment #5
daffie CreditAttribution: daffie commented+1 for the RoleInterface. It is where you would naturally expect it to be.
It all looks good to me.
Great work neclimdul!
Those horrible namespace constructions are gone.
Comment #7
daffie CreditAttribution: daffie commentedComment #8
neclimdulI think dawehner has a valid point. Referencing a module in Core is sort of like referencing a core class in Component, it exposes coupling (in this case unintended).
We had a talk in IRC and AccountInterface does sort of own the concept of this constant string. Its specifically a concept it owns an imposes on the RoleInterface so it does make some sense to put it there.
On the other hand, most of the systems interacting with this are interacting with the Role system, not the session system.
So I propose a middle ground where AccountInterface owns the constant and RoleInterface exposes it to the rest of our code.
Comment #10
neclimdulWoops, didn't look at the failure before. just assumed re-roll meant a conflict and rebased the new patch.
Comment #11
daffie CreditAttribution: daffie commentedIt looks good to me.
Personally I would have moved the role-interface to Drupal\Core\Session\RoleInterface. But I can also live with the solution in the current patch.
Comment #12
BerdirThere's still the question of disruption/BC layer I guess. Based on some recent issues/feedback from core committers, a normal task isn't allowed to make any API changes.
Given that it would be trivial to keep the old constants around, I'd suggest to do that and deprecate them with removal in 9.x.
Note that I've run into a few such constants in my issue to remove the forced load of .module files, including those I think, so this would help that issue.
Comment #13
neclimdulTestbot hasn't noticed yet but this needed a reroll. Just some chunk conflicts like a use collision.
I'm ok with either of these patches. -deprecated has the old deprecated constant and the other doesn't. I'd prefer to get rid of them but understand if we want to be strict about our Beta requirements.
Comment #15
neclimdul#2347625: Remove drupal_bootstrap and drupal_get_bootstrap_phase another chunk collision.
Deprecation is still an equal option but because it changed fewer lines it didn't have a problem with the bootstrap constant removal so I didn't re-roll it.
Comment #17
neclimdulchunk conflict shown in mergediff.
Comment #18
alexpottWe need to add the the fact the old constants are deprecated and will be removed in Drupal 9 to the old constants.
Comment #19
mrjmd CreditAttribution: mrjmd commentedAdded deprecated notes as per #18
Comment #20
neclimdulThanks!
The old constant is suppose to be an alias to the interface constant as in #13 but that got lost somehow.
Updated the @deprecated comment to point directly to the interfaces containing the constants too.
Comment #21
daffie CreditAttribution: daffie commentedCan we rename them to ANONYMOUS_RID and AUTHENTICATED_RID. They are defined in the AccountInterface and the constants are related to roles not accounts.
Comment #22
mrjmd CreditAttribution: mrjmd commentedFully renamed the new constants as suggested in #21.
Comment #24
neclimdulI disagree. The R in RID is to clarify what the ID is to be used for. In the patch the ID isn't always for roles, and the interface clarifies what the ID is for.
Comment #25
daffie CreditAttribution: daffie commented@neclimdul: And what if we rename them to AccountInterface::AUTHENTICATED_ROLE and AccountInterface::ANONYMOUS_ROLE?
Comment #26
neclimdulI'm not sure I agree but a name is a name, lets just wrap this up.
Comment #27
daffie CreditAttribution: daffie commentedLooks good to me.
Comment #28
alexpottIf we want to move to doing browser testing using PHPUnit then we have to remove all of this type of thing before release.
Therefore considering that this change allows for that and leaves a backwards compatibility layer in, I think that this change is good to make during beta.Thanks for adding the beta evaluation to the issue summary. Committed 59388d8 and pushed to 8.0.x. Thanks!
Comment #31
Mile23Edited this change record to reflect this new deprecation: https://www.drupal.org/node/1619504