OPES-1546: Allow to configure user conditions for 2FA #203
Conversation
| */ | ||
| protected static $modules = [ | ||
| 'oe_authentication_test', | ||
| 'user', |
There was a problem hiding this comment.
No need of user module
| /** | ||
| * The condition manager. | ||
| * | ||
| * @var \Drupal\Core\Executable\ExecutableManagerInterface&\Drupal\Core\Plugin\FilteredPluginManagerInterface |
There was a problem hiding this comment.
Nothing to fix here just an observation, it seems a bit odd the full namespaced classes with intersection types, extremely long.
There was a problem hiding this comment.
Ye I know but there's a bit of a mess with the ConditionManager. There's no real interface that includes all the methods of the above.
But in truth, now that I think of it, we only need the method provided by FilteredPluginManagerInterface. I will drop it.
There was a problem hiding this comment.
Or maybe @donquixote has an opinion on this?
There was a problem hiding this comment.
So I see two separate questions here.
- Code style and verbosity for union or instersection types in doc comments.
- Whether we need this intersection type or can go with a single interface.
- (not mentioned in your comments) Whether we can drop a
@vardoc type if it only repeats the native property type.
For 1. - FQCN or alias
I see plenty examples of union types (|) in core which use full FQCN.
We could come up with a OpenEuropa rule to use aliases for union and intersection types.
The main problem with alias in doc comment is that either we would sometimes add an alias only for the use in doc comments, or we have to make the doc comment conditional on whether the alias is already there for other reasons.
So for the sake of not losing sleep and time we should just put the FQCN.
(In my own side projects I have a mix of FQCN and alias, but this should not be relevant here)
For 2. - all parts of intersection type needed?
In general we should document those interfaces that are needed where we use the object.
The doc type also has informational value, but this is mostly covered by the doc description.
In core I see a mix, where ExecutableManagerInterface is used in php param type, but at least in one place ConditionManager (class) is used in the property @var doc.
There is this issue in core, 2385427: Add a ConditionManagerInterface
which is open since a while.
For 3. - Can we drop @var doc?
On the doc standards page we find
Typed class properties may omit the @var declaration. It is recommended to use typed properties whenever possible.
Seems it was introduced here: 3238192: Allow omitting @var for strictly typed class properties
Also interesting: 3376518: Allow omitting @var, @param and @return tags or their descriptions if the variable name and type is enough
Fixes #202 .