#2277547: User::getRoles test coverage & regression fix added a todo about moving the role constants from bootstrap.inc to a class. The idea is sound but I don't really see any discussion on it and there are two issues with putting this on the test.
1) a @todo on a test is not likely to be seen or acted on unless someone triggers the second problem
2) phpunit flags the test as Risky when you run in strict mode. It assumes something isn't complete about the test because you annotated it @todo.

The test is complete, just the constant might not be in the right place so the attached patch moves the @todo to the constant declaration where it will be seen. I don't know if this should be deprecated or removed entirely or what but this opens the discussion.

CommentFileSizeAuthor
#2 2432975-2.patch1.2 KBneclimdul
#2 2432975-2-interdiff.txt711 bytesneclimdul
RoleConstantTodo.patch1.16 KBneclimdul
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

What about adding an actual issue for these TODOs already?

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
FileSize
2432975-2-interdiff.txt711 bytes
2432975-2.patch1.2 KB

#2433281: Move Role Constants on to a Class/Interface

Unsure about its ability to go into 8.0.x so leaving this open and updating the patch to link to it.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

The formatting here is not OK:

 /**
  * Role ID for anonymous users; should match what's in the "role" table.
+ * @todo https://www.drupal.org/node/2433281 Move to a class/interface.
  */
 const DRUPAL_ANONYMOUS_RID = 'anonymous';

Needs a blank line before the @todo.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

#2433281: Move Role Constants on to a Class/Interface is actually RTBC, although I'm not sure if it will actually be committed like that, but we can probably close it as a duplicate?

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs work » Closed (duplicate)

*handwavy* I mostly opened this because it seemed quick and easy and I honestly didn't expect the other one to come to a consensus so quick. That said it is RTBC so I'll close this and if it gets tied up in weirdness which I don't expect that this point I'll reopen this for a quick doc fix.