# Attributes on property groups

- Thread: https://externals.io/message/111914
- Original archive: https://news-web.php.net/php.internals/111914
- Started by Nikita Popov on 2020-09-22T12:51:22+00:00
- Messages: 6
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #111914 — Nikita Popov — 2020-09-22T12:51:22+00:00

URL: https://externals.io/message/111914#111914

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

---

## #111916 — Marco Pivetta — 2020-09-22T13:04:24+00:00

URL: https://externals.io/message/111914#111916 · In reply to: #111914

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



> 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

---

## #111918 — Benas IML — 2020-09-22T13:23:14+00:00

URL: https://externals.io/message/111914#111918 · In reply to: #111916

And why would we want to do that?



> Probably easier/simpler to deprecate (then remove) property declaration
> groups, no?
>
> 
>
> > […]
> decided
> > […]
> the
> > […]

---

## #111920 — Larry Garfield — 2020-09-22T14:12:58+00:00

URL: https://externals.io/message/111914#111920 · In reply to: #111914

> 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

---

## #111923 — tyson andre — 2020-09-22T16:50:40+00:00

URL: https://externals.io/message/111914#111923 · In reply to: #111914

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

---

## #111934 — Sara Golemon — 2020-09-25T15:22:40+00:00

URL: https://externals.io/message/111914#111934 · In reply to: #111914

> 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