How should we use php8 attributes?

Hello (app) developers,

php8 is out and there is some exciting new stuff. One of them is the new attributes. I also found a neat overview/tutorial here.

The first thing where we could (or should?) use them is as a replacement for the current annotations. Annotations are the @NoAdminRequired and similar sections in the phpdoc part of a controller method.

Right now we parse those annotations with the IControllerMethodReflector.

With php8 we don’t need such a parser helper anymore. We can just define common attributes, put them onto controller methods and then use basic reflection to read the attributes in other parts of the code.

Attributes also have the advantage that they are also ordinary classes in php. So those can be part of our public API at \OCP and receive documentation and become discoverable by IDEs.

I gave this a very quick test at Use php8 attributes instead of phpdoc annotations by ChristophWurst · Pull Request #25122 · nextcloud/server · GitHub. Since my system doesn’t have php8 yet, I didn’t quite finish this, but it showed me that we have to discuss a few things first.

New attributes

Attributes are incompatible with the current annotations. Hence we can be creative with the new names, if we want to change any. Moreover, the new attributes allow parameters. So, for example, we currently have a @NoAdminRequired and a @SubAdminRequired annotation. With an attribute those could be combined into #[NoAdminRequired] with an optional parameter for the sub admin permission like #[NoAdminRequired(true)] or even fancier with a named argument like #[NoAdminRequired(allowSubAdmin: true)].

To find good names and attributes it will make sense to gather a list of the currently possible annotations. My linked PR above has four of them in it, but I’m sure I forgot some others.

Transition between annotations and attributes

Once we have new attributes in place it’s likely time to also deprecate annotations. We can most likely say we support them for the typically 3 years for which we try to guarantee backwards compatibility.

Deprecation of the controller method reflector

As mentioned the attributes are very easy to work with through PHP’s reflection mechanism. The IControllerMethodReflector is an overkill for attributes, so I’d suggest that we deprecate it and let apps use reflect (if they even need this) on their own.

Other areas

In the future we maybe also enhance other part of our code other than http controller methods.

Another thing on those controller methods would be to move routing inline to the controllers. Like instead of the routes.php file pointing to a controller method the controller method itself has an attribute with the URL and HTTP method(s) it can answer. But for this and other purposes I would suggest to wait until some standards in the php ecosystem have settled (looking at you PSR) before we do our own thing.

Versioning information like @since and @deprecated will most likely also change from annotations to attributes, but again, let’s wait until those are standardized, so that we can use the same as other libraries.

2 Likes

Thanks for the nice summary. In general I like it.

Regarding the names: I would actually stick to the ones we have as they are descriptive and also express danger like in “NoAdminRequired” or “NoSubadminRequired”. I would also not go for the parameters, because they add another layer of indirection and don’t make the code more readable or more maintainable in my opinion. Just having it written out as separate name should be fine. Also we could use inheritance in case we want to combine them somehow (NoAdminRequired could be inherited by the NoSubAdminRequired and then the base class can also be checked for without the need to specify it manually).

And good point regarding the routes and since/deprecated ones. Let’s wait here a bit and check what others do. Also as especially the latter is anyways just a documentation feature and not used during runtime there is also no urgency here to change this (from a maintenance, performance or code cleanup perspective).

1 Like

I have to disagree here. The parameters are ordinary arguments of the attribute class. Maybe admin vs sub admin was a bad example, but I don’t see a strong reason to ignore parameters. You won’t be able to express every possible combination with inheritance as inheritance gives you a static set of options while parameters allow you to plug things together dynamically.

@ChristophWurst I should want to thank for this wonderful summery. And good point regarding the routes and since/deprecated ones. I liked it so much. :slightly_smiling_face:

1 Like