Skip to content

Conversation

@cmb69
Copy link
Member

@cmb69 cmb69 commented Oct 22, 2024

I have grepped the stub files of master, and besides Deprecated attributes, which I've ignored for now, only found a couple of attributes.

So now we have:

stdclass

Great! Even linking works. However, two paragraphs above:

Despite not implementing __get()/__set() magic methods, this class allows dynamic properties and does not require the #[\AllowDynamicProperties] attribute.

Hmm.


We also have:

attribute

No linking (yet), but okay for now. Should only Attribute be linked, or also Attribute::TARGET_CLASS?


And now:

deprecated

Okay, there is a scroll bar at the bottom of the classsynopsis, and the view width isn't up to modern standards (maybe), but this doesn't look great. What to do here? Leave as is? More line breaks? Skip the Attribute:: "prefix"? The latter would look like

deprecated2

And how to handle Deprecated. We already have prominent warnings about deprecations, and even versions.xml allows to specify that. Still add the attribute?

It seems to me that before even implementing documentation support for attributes in gen_stub.php, we need to figure out the details. Suggestions welcome!

@jimwins
Copy link
Member

jimwins commented Oct 23, 2024

I think the text about stdClass not requiring the #[AllowDynamicProperties] attribute should just be reworked if the attribute is added to the definition of the class. It has the attribute, and any class that inherits from it inherits that, but stdClass is not a base class that all classes inherit, which is the important point to be made.

We should link the attribute's class name. Linking arguments like Attribute::TARGET_CLASS would be nice, but there's not much we can do inside a <modifier> like mark those up with <constant>. I'm fine leaving those unlinked until there's actually more complicated usage of attributes that needs to be documented. Since the constants are from the same class we're already linking, it seems obvious enough.

I don't think we should drop the class name on the constants, we'll just have to deal with the overflow/wrapping some other way. Throwing spaces around the '|' would be good (and is probably coding style compliant?).

I wouldn't include Deprecated attributes, that should be documented through the existing warnings.


<classsynopsis class="class">
<ooclass>
<modifier role="attribute">#[Attribute(Attribute::TARGET_CLASS)]</modifier>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gen_stub adds the trailing backslash:

Suggested change
<modifier role="attribute">#[Attribute(Attribute::TARGET_CLASS)]</modifier>
<modifier role="attribute">#[\Attribute(Attribute::TARGET_CLASS)]</modifier>

Although unfortunately it doesn't (yet) handle parameters :/

@kocsismate
Copy link
Member

And how to handle Deprecated. We already have prominent warnings about deprecations, and even versions.xml allows to specify that. Still add the attribute?

Personally, I'm fine with adding them, purely because of consistency. It doesn't hurt to have yet another way to display the deprecation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants