Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Binary string forward compatibility removal#2187

Closed
pmmaga wants to merge 8 commits into
php:masterphp/php-src:masterfrom
pmmaga:binary-string-removalpmmaga/php-src:binary-string-removalCopy head branch name to clipboard
Closed

Binary string forward compatibility removal#2187
pmmaga wants to merge 8 commits into
php:masterphp/php-src:masterfrom
pmmaga:binary-string-removalpmmaga/php-src:binary-string-removalCopy head branch name to clipboard

Conversation

@pmmaga
Copy link
Copy Markdown
Contributor

@pmmaga pmmaga commented Nov 6, 2016

In version 5.2.1, the b prefix and the (binary) cast were introduced for forward compatibility with the PHP6 project. As it is known, that project never came to be. However, these are still accepted by the language scanner although ignored from then on.

This PR aims at removing those as they are naturally confusing given that they are simply ignored or, in the case of the cast, the same as casting to string.

However, I've separated this PR in various commits because the removal of the (binary) cast will bring a more serious BC break issue: On the PHAR extension, the current default stub for PHAR's makes use of the binary cast. I've removed it from the default stub and fixed the tests that made use of it but this means that the old PHARs that make use of this default stub will be broken.

Also, I have split the tests that were using the prefix or the cast but were unrelated to them from the others as these may be introduced first if this is split up in multiple steps.

What seemed to be a simple task actually turned out to be far more complex given the number of tests that were written with this forward compatibility in mind.

I would be happy to transform this into an RFC if you think it would be appropriate.

@smalyshev smalyshev added the RFC label Nov 6, 2016
@KalleZ
Copy link
Copy Markdown
Member

KalleZ commented Nov 6, 2016

Instead of changing phpt's to have this in their name:

+no longer support basic binary nowdoc syntax

Same goes for the whole tests, where the EXPECT part have changed to just a parse error, either the tests should be updated to work without the b-prefix or the (binary) cast, or dropped entirely.

@pmmaga
Copy link
Copy Markdown
Contributor Author

pmmaga commented Nov 6, 2016

I did that for the heredoc and nowdoc tests on Zend/ because they were specific for the prefix (otherwise identical to _001) and I didn't want to leave a hole or change the numeration for historical reasons. Should I?

Related to this and other feedback I'll try to split these changes in smaller commits.

Thanks

@KalleZ
Copy link
Copy Markdown
Member

KalleZ commented Nov 6, 2016

Hi @pmmaga its fine that the numeration does not fit, we do have a few cases like this around the source, and after all, it is just an irrelevant number :)

@KalleZ
Copy link
Copy Markdown
Member

KalleZ commented Nov 6, 2016

Also, as a quick reply, this would need an RFC, and since it changes the syntax, it would need a 2/3+1 vote to be accepted :)

@pmmaga pmmaga force-pushed the binary-string-removal branch from 6269606 to a22e013 Compare November 6, 2016 22:23
@pmmaga
Copy link
Copy Markdown
Contributor Author

pmmaga commented Nov 6, 2016

@KalleZ , thanks for the tips. I've removed those tests and split up the commits a bit better.

@KalleZ
Copy link
Copy Markdown
Member

KalleZ commented Nov 7, 2016

Like I just mentioend in PR #2190, then at least the phar stub binary casts should go into 7.1 if possible.

@dshafik @krakjoe

@nikic
Copy link
Copy Markdown
Member

nikic commented Nov 7, 2016

Agree, we should merge the phar stub fix asap.

We should also merge the removal of b/binary from unrelated tests in master to avoid bitrot of this huge patch. The actual removal/deprecation/whatever can then be handled later much more easily.

@pmmaga
Copy link
Copy Markdown
Contributor Author

pmmaga commented Nov 7, 2016

Ok, that sounds good. Should I create individual PRs for each thing?

@pmmaga
Copy link
Copy Markdown
Contributor Author

pmmaga commented Nov 24, 2016

I'll close this PR as this change should go through a deprecation phase before removal.

@pmmaga pmmaga closed this Nov 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.