Skip to content

[String] Check if function exists before declaring it#40203

Merged
nicolas-grekas merged 1 commit intosymfony:5.2from
Nyholm:string-function-exists
Feb 16, 2021
Merged

[String] Check if function exists before declaring it#40203
nicolas-grekas merged 1 commit intosymfony:5.2from
Nyholm:string-function-exists

Conversation

@Nyholm
Copy link
Copy Markdown
Member

@Nyholm Nyholm commented Feb 16, 2021

Q A
Branch? 5.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

If you installed a command line tool like psalm with composer and then try to run it on a project that included the String component you will get an error like:

Fatal error: Cannot redeclare Symfony\Component\String\u() (previously declared in /Workspace/symfony/src/Symfony/Component/String/Resources/functions.php:14) in /user/.composer/vendor/symfony/string/Resources/functions.php on line 14

That is because we are loading two installations of the string component.

Comment thread src/Symfony/Component/String/Resources/functions.php Outdated
@Nyholm
Copy link
Copy Markdown
Member Author

Nyholm commented Feb 16, 2021

I've updated the code to use ::class, I've also fixed the same issue in Translation component

@nicolas-grekas
Copy link
Copy Markdown
Member

Thank you @Nyholm.

@nicolas-grekas nicolas-grekas merged commit 7dcf156 into symfony:5.2 Feb 16, 2021
@Nyholm
Copy link
Copy Markdown
Member Author

Nyholm commented Feb 16, 2021

Thank you for merging.

@Nyholm Nyholm deleted the string-function-exists branch February 16, 2021 10:21
@fabpot fabpot mentioned this pull request Mar 4, 2021
@weirdan
Copy link
Copy Markdown

weirdan commented Mar 6, 2021

If you installed a command line tool like psalm
[...]
I've updated the code to use ::class

Ironically, this broke Psalm tests: https://github.com/vimeo/psalm/runs/2037304361#step:7:42

I would suggest to replace

if (!\function_exists(u::class)) {

with

if (!\function_exists(__NAMESPACE__ . '\\u')) {

@Nyholm
Copy link
Copy Markdown
Member Author

Nyholm commented Mar 6, 2021

The current implementation is valid PHP code. I’m not sure what the benefits are to implement to your suggestion.

I don’t want to be disrespectful, but this is a problem psalm should fix, isn’t it?

@weirdan
Copy link
Copy Markdown

weirdan commented Mar 6, 2021

I’m not sure what the benefits are to implement to your suggestion.

Clearer code, mostly. I had this little 'huh?' moment looking at it, thinking 'What are those weirdly named classes? And why are they used in function_exists()? Oh, those are not classes...'

this is a problem psalm should fix, isn’t it?

Perhaps. Hardly a high-priority issue though, as in practice it will only happen when people run Psalm with badly written custom autoloader.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants