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

NewLanguageConstructs: Update the version number for T_COALESCE_EQUAL #523

Merged
merged 1 commit into from
Nov 2, 2017

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Oct 25, 2017

While this language construct is already tokenized correctly in PHPCS, PHP itself has not yet implemented it.

The RFC has been approved a while back and it was slated to go into PHP 7.2, but as of PHP 7.2 RC4 no implementation has been merged and it is not expected to go into PHP 7.2 anymore.

As PHPCS already recognized the token, it was already added to the NewLanguageConstructs sniff a while back.

All this PR does is up the PHP version number used for the compares.

While this language construct is already tokenized correctly in PHPCS, PHP itself has not yet implemented it.

The RFC has been approved a while back and it was slated to go into PHP 7.2, but as of PHP 7.2 RC4 no implementation has been merged and it is not expected to go into PHP 7.2 anymore.
@jrfnl jrfnl requested a review from wimg October 25, 2017 06:11
@wimg wimg merged commit 7df89cb into master Nov 2, 2017
@wimg wimg deleted the php-7.2/lang-construct-update-coalesce-equals branch November 2, 2017 01:15
@jrfnl jrfnl added this to the 8.1.0 milestone Nov 13, 2017
@jrfnl jrfnl added the chores/QA label Mar 8, 2018
@MarkMaldaba
Copy link
Contributor

In general, we should avoid adding features from RFCs until they're actually released...

@wimg
Copy link
Member

wimg commented Mar 25, 2018

I agree that as long as the code has not been merged in any branch of the PHP source code, having this sniff might not be ideal. Then again, in this specific case it will not cause any issues, so we could leave it in there ?

@jrfnl
Copy link
Member Author

jrfnl commented Mar 25, 2018

In general, we should avoid adding features from RFCs until they're actually released...

I agree that as long as the code has not been merged in any branch of the PHP source code, having this sniff might not be ideal.

@MarkMaldaba @wimg I totally agree. However, this is a bit of a special case.

History:
I'd seen the RFC which was slated to go into PHP 7.2 and noticed it was going to introduce a new token, so I gave PHPCS upstream a heads-up in advance as a sort of reminder ticket.

The idea was that the upstream ticket would remain open until the RFC was merged into PHP and that the token could then be implemented into PHPCS quickly.
This would save us the trouble of having to account for three instead of two different situations (1+3):

  1. Token doesn't exist;
  2. Token exists, but not in older PHP versions;
  3. Token exists + is backfilled for older PHP versions correctly by PHPCS.

To my surprise, the token back-fill was then implemented into PHPCS straight away, even though the token was not in PHP yet.
See: squizlabs/PHP_CodeSniffer@a668152 (Feb 2017)

As PHPCS already added the token, I figured it made sense to add it here as well, which is why I pulled it in #340.

Then again, in this specific case it will not cause any issues, so we could leave it in there ?

So, yes, as the token already exists in PHPCS, I think leaving it in, makes the most sense.

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.

3 participants