Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add attributes #3897

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add attributes #3897

wants to merge 1 commit into from

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.

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.

2 participants