Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
user.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
24 Feb 2015 at 04:50 UTC
Updated:
10 Nov 2015 at 23:13 UTC
Jump to comment: Most recent, Most recent file
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
\DrupalorAccountInterfaceis a better place for it?Comment #5
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 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 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 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 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 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 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 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