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

Attribute parameters cannot contain BackedEnum values #8344

Closed
michnovka opened this issue Apr 11, 2022 · 18 comments
Closed

Attribute parameters cannot contain BackedEnum values #8344

michnovka opened this issue Apr 11, 2022 · 18 comments

Comments

@michnovka
Copy link

Description

The following code:

<?php

enum Status: string
{
    case DRAFT = 'draft';
    case PUBLISHED = 'published';
    case ARCHIVED = 'archived';
}

#[Attribute]
class ListensTo
{
    public string $event;

    public function __construct(string $event)
    {

        $this->event = $event;
    }
}

#[ListensTo(Status::DRAFT->value)]
class TestClass {}

Resulted in this output:

PHP Fatal error:  Constant expression contains invalid operations on line 23

But I expected it would work fine

PHP Version

PHP 8.1.4

Operating System

Ubuntu 20.04

@damianwadley
Copy link
Member

->value isn't a constant expression: it requires a class instance to get the property from.

Why not use the enum properly? https://3v4l.org/BCqrS

@michnovka
Copy link
Author

michnovka commented Apr 11, 2022

@damianwadley I tried to make the example as minimal as possible. The reason why not use the enum directly is that I need it as an array key for my implementation

<?php

enum Status: string
{
    case DRAFT = 'draft';
    case PUBLISHED = 'published';
    case ARCHIVED = 'archived';
}

#[Attribute]
class ListensTo
{
    public array $events;

    public function __construct(array $events)
    {

        $this->events = $events;
    }
}

#[ListensTo([Status::DRAFT->value => true])]
class TestClass {}

This Attribute is from a 3rd party library and I cannot change it. The key values correspond to values of enum cases I have specified elsewhere in the project. So now I have to have "magic strings" like

#[ListensTo(['draft' => true])]

And when I change Status::DRAFT, I need to change it on more places.

@iluuu1994
Copy link
Member

There's two ways to solve this:

  1. Allow -> in constant expressions
  2. Allow enums (or objects) as array keys (https://wiki.php.net/rfc/object_keys_in_arrays)

Solution 1 didn't make sense before enums and before allowing new in constant expressions but I guess at this point it would be somewhat more sensible. @nikic WDYT?

@nikic
Copy link
Member

nikic commented Apr 11, 2022

Yeah, I think supporting -> in constant expressions makes sense.

@michnovka
Copy link
Author

Can we target this as a bug for 8.1?

@iluuu1994
Copy link
Member

@michnovka This is not a bug but a limitation. -> were never supported for constant expressions. New features only land in new minor/major versions of PHP.

@michnovka
Copy link
Author

Alright. Do we need some proper vote on this? Or is my job done here and you guys will now take over? :)

@bwoebi
Copy link
Member

bwoebi commented Apr 12, 2022

@iluuu1994 There are instances where such limitations (especially when discovered in the version they were introduced in) these were fixed within that version.

Given that implementing this is not breaking at all (I suppose), there is a reasonable case to just fix it.

But I'll leave it at your discretion.

@michnovka
Copy link
Author

Even if it were just for enums, the Enum::CASE->value is also always a constant, so we are not allowing some non-constant expressions this way. Is there any other place where -> could be used in constant expressions? I don think so

@iluuu1994
Copy link
Member

@michnovka (new Foo)->bar

@bwoebi I'd be more comfortable in a minor/major, especially if changes are bigger. What would be worse is releasing it in a patch, then realizing there's some issue and removing it again.

@michnovka
Copy link
Author

michnovka commented Apr 12, 2022

@iluuu1994 I dont know how Enum::CASE is handled internally in constant expressions now. Is it instantiated? If not, I would very much like to fix it just for Enums in this version and make the big change of allowing all -> in major. After all, I just need to access a "static property" of BackedEnum. It feels like a strong limitation that should not be mixed with assigning (new Foo)->bar to constant expressions

What I mean to say is that the syntax might as well have been Enum::CASE::value but you went with Enum::CASE->value. value is static property tho.

@iluuu1994
Copy link
Member

iluuu1994 commented Apr 12, 2022

@michnovka It's not easier if we allow -> just for enums, the implementation is the same.

What I mean to say is that the syntax might as well have been Enum::CASE::value

-> is used to access instance properties, that's what name is.

@michnovka
Copy link
Author

michnovka commented Apr 12, 2022 via email

@iluuu1994
Copy link
Member

@michnovka Well, special casing enums here doesn't make sense IMO, especially not just to allow this edge case. And I don't think that would be easier either. I think it's fair for this to wait for PHP 8.2.

@cmb69
Copy link
Member

cmb69 commented Apr 12, 2022

cc @Crell

@Crell
Copy link
Contributor

Crell commented Apr 12, 2022

I can't think of a good language argument to not allow En::Case->value (and I suppose also En::Case->name) in constant expressions. I could argue that it's a design flaw in an API, but that's a separate matter. If Nikita thinks it's safe them I'm fine with allowing it.

I agree with others here that this sounds like an 8.2 feature. I also don't see a good reason to limit it to enums, when enums are really just sugar over objects to begin with. (new Foo)->value is very close to what's actually going on anyway.

So yeah, my vote would be for allowing static -> de-references in constant expressions generally, as of 8.2. That... probably requires an RFC.

@iluuu1994
Copy link
Member

I created a PoC for this here. I'll start the RFC process soon.

@ramsey ramsey linked a pull request May 24, 2022 that will close this issue
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Jun 23, 2022
@iluuu1994
Copy link
Member

Given there's an RFC for this now I'm closing this issue. Thanks!

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

Successfully merging a pull request may close this issue.

7 participants