Everything

php

Home » Project » Newsgroups » php.internals » Attributes on property groups »

Attributes on property groups

php.internals

Nikita Popov

5 years ago

Hi internals,

Currently, placing an attribute on property (or constant) groups is not
allowed:

class Foo {
#[NonNegative]
public int $x, $y, $z;
}
// Fatal error: Cannot apply attributes to a group of properties

This is a case that was not explicitly mentioned in the RFC and we decided
to be conversative when landing the initial implementation.

However, this restriction seems pretty arbitrary to me, and I think we
should remove it. While there is some potential ambiguity as to whether the
attribute applies to all properties or only the first one, I think the
general expectation is that it should apply to all properties, just like
the property type does.

PR to allow this: https://github.com/php/php-src/pull/6186

Any thoughts on this?

Regards,
Nikita

Marco Pivetta

5 years ago

Probably easier/simpler to deprecate (then remove) property declaration
groups, no?

On Tue, Sep 22, 2020, 14:51 Nikita Popov <nikita.ppv@gmail.com> wrote:

Benas IML

5 years ago

And why would we want to do that?

On Tue, Sep 22, 2020, 4:04 PM Marco Pivetta <ocramius@gmail.com> wrote:

Larry Garfield

5 years ago

On Tue, Sep 22, 2020, at 7:51 AM, Nikita Popov wrote:

> Hi internals,
>
> Currently, placing an attribute on property (or constant) groups is not
> allowed:
>
> class Foo {
>     #[NonNegative]
>     public int $x, $y, $z;
> }
> // Fatal error: Cannot apply attributes to a group of properties
>
> This is a case that was not explicitly mentioned in the RFC and we decided
> to be conversative when landing the initial implementation.
>
> However, this restriction seems pretty arbitrary to me, and I think we
> should remove it. While there is some potential ambiguity as to whether the
> attribute applies to all properties or only the first one, I think the
> general expectation is that it should apply to all properties, just like
> the property type does.
>
> PR to allow this: https://github.com/php/php-src/pull/6186
>
> Any thoughts on this?
>
> Regards,
> Nikita

I don't know if it's safe to change so close to RC; I'd be fine with leaving it for a version since grouping is in practice so rare.  If you decide to change it now, however, I agree that applying to all items in the group is the least-surprising approach.

--Larry Garfield

tyson andre

5 years ago

Hi Nikita,

> Hi internals,
> 
> Currently, placing an attribute on property (or constant) groups is not
> allowed:
> 
> class Foo {
>     #[NonNegative]
>     public int $x, $y, $z;
> }
> // Fatal error: Cannot apply attributes to a group of properties
> 
> This is a case that was not explicitly mentioned in the RFC and we decided
> to be conversative when landing the initial implementation.
> 
> However, this restriction seems pretty arbitrary to me, and I think we
> should remove it. While there is some potential ambiguity as to whether the
> attribute applies to all properties or only the first one, I think the
> general expectation is that it should apply to all properties, just like
> the property type does.
> 
> PR to allow this: https://github.com/php/php-src/pull/6186
> 
> Any thoughts on this?

That seems like a good idea and doesn't seem like an ABI change. I'm surprised this was a special case in the original implementation.
I wouldn't consider it a BC break if code that was previously a CompileError no longer threw a CompileError.

The attribute node occurs before property modifiers such as `private static`, which already pretty clearly apply
to all elements in an attribute group, not just the first element.

Regards,
- Tyson

Sara Golemon

5 years ago

On Tue, Sep 22, 2020 at 7:51 AM Nikita Popov <nikita.ppv@gmail.com> wrote:

> Currently, placing an attribute on property (or constant) groups is not
> allowed:
>
> This is a case that was not explicitly mentioned in the RFC and we decided
> to be conversative when landing the initial implementation.
>
>

Conservative is a good default, but as Tyson pointed out, nobody is
confused about visibility modifiers applying to the entire group, I don't
believe they'd be confused about attribute application.
((Though I am curious what phpdoc syntax says here.))

> However, this restriction seems pretty arbitrary to me, and I think we
> should remove it. While there is some potential ambiguity as to whether the
> attribute applies to all properties or only the first one, I think the
> general expectation is that it should apply to all properties, just like
> the property type does.
>

Agreed. The limitation seems arbitrary and unnecessary.  Call this a
refinement of the feature previously landed, and let's get it into 8.0.

-Sara