-
-
Notifications
You must be signed in to change notification settings - Fork 131
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
Implement Stringable interface on Enum #160
Conversation
Hey hey, thanks! I'd love to skip requiring the polyfill: this package is used everywhere, and polyfills add extra (often useless) code to download and packaged applications. I'm torn between:
How is the Stringable useful in practice? |
Maybe another possibility is to implement the polyfill directly in the library, it's just a single interface with a classmap definition for the autoloading.
In a project I work on, I use a library which types a lot of arguments method with the |
👍 I'm actually +1 on re-implementing the polyfill interface. That's usually stuff I don't want to maintain, but here given the impact of this project I'd rather not take any risk. |
I've just updated the PR |
LGTM |
Hi @mnapoli, do you have an opinion on this PR ? Just to know, if I should closed it or not :) |
@@ -0,0 +1,11 @@ | |||
<?php | |||
|
|||
if (\PHP_VERSION_ID < 80000 && !interface_exists('Stringable')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
symfony/polyfill-php80
has no if condition there: https://github.com/symfony/polyfill-php80/blob/main/Resources/stubs/Stringable.php
Is there a way to control what would composer use in the classmap for this interface as a file? I guess it should be only 1 file and it doesn't matter which one of them will get here if both would be available.
Do you see a issue where symfony might be included after this one and having no if condition it would fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good point 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't matter which one is used as both polyfill "emulate" the same interface.
The classmap autoloads will result in an associative array which associate a class with a file which implement this one.
From the Composer autoload view, only one file will be loaded.
Sorry for the delay, I lost track of it. Let's do this. Thanks @jdecool |
Enum class has a
__toString
, so it would be great to implements the newStringable
interface.As the interface is not available for php < 8, I've
had theimplemented a polyfill.symfony/polyfill-php-80