Check PHP version in stubs classes

[jdeniau]

I added a test in stubs classes to prevent php -l to fail on polyfills (errors like PHP Fatal error: Cannot declare class ArithmeticError, because the name is already in use)

• add test to lint polyfill files
• make lint test pass

[keradus]

👎 on that,
stubs are not loaded if they are not needed, eg, if we are on php7, we have TypeError defined, thus TypeError.php is not autoloaded at all. If we are on php5, TypeError is not defined by language, thus TypeError.php is loaded and in will always have to register the class.
that means, added conditions will always execute as true

[jdeniau]

@keradus You are right, but if you want to write a wordpress plugin, the wordpress svn pre-commit hook will simply reject your plugin because symfony/polyfill-php70 is included in the vendors 😞

As you said, stubs will be included only on older version of PHP, and only one time, so I think the "cost" of an if (true) is really insignificant versus the developper experience here.

[keradus]

if wordpress has issues how to properly deal with code, it shall be fixed on wordpress level, if you ask me

[jdeniau]

Of course, that's the proper way, and I already asked as you can imagine.

But I don't think it's only a wordpress issue, or I would not have opened a PR (I already forked polyfill-php70 and used it in my composer file), as linting all file of a directory to check errors is something often done.
I quoted Wordpress as an example here.

[nicolas-grekas]

I agree with @keradus, this is working around someone else's issue.

[stof]

When relying on autoloading, a class will not be autoloaded if it already exists in the runtime.

So the issue is in the infra running php -l on any file without taking things like that into account, not in code relying on the way PHP works.

[nicolas-grekas]

Thanks for proposing.

[karser]

Encountered this issue. Unfortunately, removing symfony/property-access from the plugin.

[nickyoung87]

I am also having this issue when pushing to SVN for the WP plugin repo. The same exact issue as @karser

What's the workaround here?

[TimothyBJacobs]

if wordpress has issues how to properly deal with code, it shall be fixed on wordpress level, if you ask me

I don't think this is WordPress doing anything improper. It doesn't seem unreasonable to me to expect that all files in a project are valid according to PHP's own built-in linter.

[bshaffer]

@nickyoung87 @TimothyBJacobs
I added a workaround (and instructions) in this gist. Apply that patch to your svn branch to fix the issue! (via @jdeniau)

Note: This is only for polyfill-php70, a different patch file would need to be created for polyfill-php56

[nickyoung87]

Thanks @bshaffer! My only problem with this as a solution is now I have to remember to make that change if this package ever gets updated (and the fix is not included). Hopefully that doesn't happen :D

[Spreeuw]

as an alternative to applying a diff like @bshaffer does, you could also iterate over the files using bash. I'm using this in my github actions when deploying:

for filename in *.php; do
    [ -e "$filename" ] || continue
    sed -i '2s/^/if (PHP_VERSION_ID < 70000) :\n/' "$filename"
    sed -i '$s/$/\nendif;/' "$filename"
done

but of course you can run this locally too