BCFile::getMemberProperties(): sync with upstream

[DanielEScherzer]

PHPCS 3.12.0 adds support for reporting if properties are final; polyfill the upstream method and copy over the tests.

Ref: PHPCSStandards/PHP_CodeSniffer#834
Closes #645

[DanielEScherzer]

No idea why this is failing, apparently the actual results don't include is_final? But running against 8.4.3 locally the tests pass. I thought I would try to contribute to the back-compat utils but I'm not sure I'll be able to get this working anytime soon

[jrfnl]

@DanielEScherzer Thank you for working on this.

No idea why this is failing, apparently the actual results don't include is_final? But running against 8.4.3 locally the tests pass. I thought I would try to contribute to the back-compat utils but I'm not sure I'll be able to get this working anytime soon

That's my bad. There should be a CONTRIBUTING file, but I never got round to writing it.

Would you like me to coach you through this ? or would you prefer I take over and make the additional changes needed ?

The short of it, is like this (with some historic context):

• Originally this library supported a wide range of PHPCS versions, with the BC layer (the files in the BackCompat subspace) basically providing polyfills for the latest versions of the PHPCS native utility functions.
  Since then, the policy for this has changed and the library will now only polyfill when there are no tokenizer changes to account for (like in the case of this polyfill).
  For more historic context about this decision, see: Drop support for PHPCS < 3.7.1 #347 and Proposal: drop support for PHPCS < 3.7.0/3.7.1 PHPCompatibility/PHPCompatibility#1347
  In a future major release, the BC layer may well get removed altogether.
• Also, originally (before I took over maintenance of PHPCS), it could take a long time for PRs to PHPCS itself to get merged, let alone released, while external standards did want to start supporting new PHP syntaxes in their sniffs (or have certain bugs fixed).
  So, at the time, while the BC layer could not support certain syntaxes (yet, as the BC layer had to be completely in line with PHPCS itself), the PHPCSUtils native methods could support those syntaxes already.
  So, there are PHPCSUtils native versions for a number of the PHPCS utility methods which would (in the past) often be ahead of PHPCS itself in supporting new PHP syntaxes and may contain some other bug fixes and improvements which never made it into PHPCS (as it took too long to get them merged).
  For those PHPCS methods for which this applies, the BCFile class contains @see tags in the function docblocks.
• The PHPCSUtils native "mirror"-methods would generally support everything PHPCS supported + some extras. To safeguard this, the tests for those methods are closely tied to the tests for the BCFile methods.
  👉🏻 This is where things go wrong for this PR. The BCFile::getMemberProperties() methods has been updated, but the mirror-method Variables::getMemberProperties() has not been updated, which is why the tests are failing.

Does that help explain things well enough ?

Some additional context:
At some point in the future, I would like to try and get rid of the duplication, so I'm playing with the idea of making PHPCSUtils a run-time dependency for PHPCS itself and deprecating the PHPCS native utility methods in favour of the ones in PHPCSUtils.
That would also solve some long-standing bugs in PHPCS itself by making solutions contained in PHPCSUtils available to the PHPCS native sniffs.
Having said that, there are more urgent problems to address in PHPCS itself, so this idea is for the long term, possibly PHPCS 5.0.

[DanielEScherzer]

For those PHPCS methods for which this applies, the BCFile class contains @see tags in the function docblocks.

Sorry - the doc block was so long that I missed it

This is where things go wrong for this PR. The BCFile::getMemberProperties() methods has been updated, but the mirror-method Variables::getMemberProperties() has not been updated, which is why the tests are failing.

Okay, I was thrown by the

/home/runner/work/PHPCSUtils/PHPCSUtils/Tests/BackCompat/BCFile/GetMemberPropertiesTest.php:74
phpvfscomposer:///home/runner/work/PHPCSUtils/PHPCSUtils/vendor/phpunit/phpunit/phpunit:60

for the location of the failure in the github actions logs. I've updated Variables::getMemberProperties() and Tests/Utils/Variables/GetMemberPropertiesTest.php now passes locally. Hopefully this will now pass [narrator: famous last words]

At some point in the future, I would like to try and get rid of the duplication, so I'm playing with the idea of making PHPCSUtils a run-time dependency for PHPCS itself and deprecating the PHPCS native utility methods in favour of the ones in PHPCSUtils.
That would also solve some long-standing bugs in PHPCS itself by making solutions contained in PHPCSUtils available to the PHPCS native sniffs.

Sounds interesting, though I would ask why not just merge PHPCSUtils into PHPCS if

• PHPCSUtils cannot really be used with PHPCS (from what I understand)
• PHPCS is going to depend on PHPCSUtils being around
  ?

[jrfnl]

This is where things go wrong for this PR. The BCFile::getMemberProperties() methods has been updated, but the mirror-method Variables::getMemberProperties() has not been updated, which is why the tests are failing.

I've updated Variables::getMemberProperties() and Tests/Utils/Variables/GetMemberPropertiesTest.php now passes locally. Hopefully this will now pass [narrator: famous last words]

Nearly there. There is one more thing to account for: PHPCSUtils supports both PHPCS 3.x as well as 4.x and PHPCS 4.0 will contain a tokenizer change related to the PHP 8.0 "namespaced names as one token" change, so the expectations for some of the tests need to take this into account, like 'type_token' => ($php8Names === true) ? -8 : -9, (there are more examples in the BCFile::GetMemberPropertiesTest class file if you need them).

At some point in the future, I would like to try and get rid of the duplication, so I'm playing with the idea of making PHPCSUtils a run-time dependency for PHPCS itself and deprecating the PHPCS native utility methods in favour of the ones in PHPCSUtils.
That would also solve some long-standing bugs in PHPCS itself by making solutions contained in PHPCSUtils available to the PHPCS native sniffs.

Sounds interesting, though I would ask why not just merge PHPCSUtils into PHPCS if

• PHPCSUtils cannot really be used with PHPCS (from what I understand)
• PHPCS is going to depend on PHPCSUtils being around

I'm not sure I understand the first bullet ?

What is now PHPCSUtils, was originally proposed to go into PHPCS itself: squizlabs/PHP_CodeSniffer#2189, but by the time I was asked to redo 160+ commits for the third time, I called it quits and published it as PHPCSUtils.

Merging Utils into PHPCS itself has various downsides, which is why I'm not keen on that. Making PHPCSUtils a runtime dependency for PHPCS would not have the same downsides. Think:

• As per the test failure remark above - PHPCSUtils currently supports PHPCS 3.x as well as 4.x. The advantage of that is that external standards using PHPCSUtils need to do less work to get ready for PHPCS 4.0, as a lot of what they would need to do is done via PHPCSUtils.
  If Utils was merged into PHPCS itself, this cross-version compatibility support would be stripped out.
  Of course, once Utils drops support for PHPCS 3.x, it will be stripped out too, but external standards can indicate a range of PHPCSUtils versions which are acceptable for them, so they could still benefit from the cross-version support.
• Utils now has its own releases, not bound directly to a PHPCS release. I see benefits in that.
• PHPCS itself is basically a lot of different packages in one package. Adding yet one more to that is not going to make maintenance more straight-forward.
  In reality, if I would build PHPCS from scratch, I would split a number of parts of it off to separate packages. Unfortunately, doing so now would be a huge breaking change, so is not on the table at this time.
• Etc

[jrfnl]

@DanielEScherzer Just checking in, I'd love to get this merged, but the tests are still failing on PHPCS 4.0. Will you have a chance to update the PR for this ?

[DanielEScherzer]

@DanielEScherzer Just checking in, I'd love to get this merged, but the tests are still failing on PHPCS 4.0. Will you have a chance to update the PR for this ?

Sorry, I tried to figure out updating this, but I'm not sure how to get things to pass against both versions of PHPCS (3 and 4) with both PHP 7.4 tokenization and 8.0+ tokenization. I figured I would try to contribute my upstream changes to this repo too, but I don't understand enough about this repo to debug more - should I just close this?

[jrfnl]

Sorry, I tried to figure out updating this, but I'm not sure how to get things to pass against both versions of PHPCS (3 and 4) with both PHP 7.4 tokenization and 8.0+ tokenization. I figured I would try to contribute my upstream changes to this repo too, but I don't understand enough about this repo to debug more - should I just close this?

No worries. If it's okay with you, I'll just add an extra commit with the necessary changes so you can see what was needed.

[Edit]: Oh and I'd be happy to answer any questions you have about this repo. Happy to support you in your contributions.

[jrfnl]

@DanielEScherzer I've added an extra commit which fixes things up. I'll merge this once the build has passed (hoping I didn't miss anything).