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

[RFC] Deprecate PHP's Short Open Tags in PHP 7.4 #3972

Closed
wants to merge 11 commits into from

Conversation

@Girgias
Copy link
Contributor

Girgias commented Mar 21, 2019

Implementation of my RFC for deprecating PHP Short tags: https://wiki.php.net/rfc/deprecate_php_short_tags

First time making a PR so help/reviews are appreciated.

On my local machine, some tests fail but I suppose this is due to me using WSL and not a native Linux.

Also, I'm having trouble fixing two tests related to strip_tags:

  • ext/standard/tests/strings/strip_tags_variation5.phpt
  • ext/standard/tests/strings/strip_tags_variation9.phpt

And it seems I made some changes to the output which I only realized with the diff view (don't know if that's due to my IDE or being on WSL).
Fixed.

Probably gonna do a second PR for the removal in PHP 8.

@petk petk added the RFC label Mar 21, 2019
@Prasitt

This comment has been minimized.

Copy link

Prasitt commented Mar 22, 2019

#3972

@Girgias Girgias changed the title [WIP][RFC] Deprecate PHP Short tags in PHP 7.4 [RFC] Deprecate PHP Short tags in PHP 7.4 Mar 22, 2019
@Girgias

This comment has been minimized.

Copy link
Contributor Author

Girgias commented Mar 22, 2019

I think this should be ready for review.
However I'm having trouble compiling PHP on my local WSL machine with all extensions to check how it affects those, I can't seem to make the SQLite3 extension work.

@Girgias Girgias changed the title [RFC] Deprecate PHP Short tags in PHP 7.4 [RFC] Deprecate PHP's Short Open Tags in PHP 7.4 Mar 25, 2019
@Girgias Girgias force-pushed the Girgias:PHP-7.4 branch from 33fbd04 to f25c36b Apr 13, 2019
php.ini-development Outdated Show resolved Hide resolved
php.ini-development Show resolved Hide resolved
@@ -43,6 +43,7 @@ foreach($string_array as $string)
echo "Done";
?>
--EXPECT--
Deprecated: Directive 'short_open_tag' is deprecated in Unknown on line 0

This comment has been minimized.

Copy link
@pmmaga

pmmaga Apr 24, 2019

Contributor

For most of these tests, that ini entry doesn't really make a difference. I'd suggest to remove it from those tests instead of adding the deprecation notice.

This comment has been minimized.

Copy link
@Girgias

Girgias Apr 24, 2019

Author Contributor

I left the deprecation notices in the tokenizer tests, should I remove those too?

This comment has been minimized.

Copy link
@pmmaga

pmmaga Apr 24, 2019

Contributor

I'd say it is safe to remove from token_get_all_variation15.phpt given that the test doesn't even use the short open tags. For 002.phpt you'd have to check if it actually influences the behavior of the tokenizer.

This comment has been minimized.

Copy link
@Girgias

Girgias Apr 24, 2019

Author Contributor

The diff for 002 is pretty large as the behavior is indeed changes as can be seen with my PR for the removal in PHP 8 #3975

c.f. https://github.com/php/php-src/pull/3975/files#diff-55fd96e8ffa1d4e3150d3eb9dbc52829

This comment has been minimized.

Copy link
@pmmaga

pmmaga Apr 24, 2019

Contributor

Makes sense. On that PR, it would probably be more useful for that test to switch from <? to <?php to maintain its usefulness. As it stands right now, 2/3 of the things being tested are lost.

This comment has been minimized.

Copy link
@Girgias

Girgias Apr 24, 2019

Author Contributor

Yeah, was probably thinking that changing the opening tag would be better than changing the whole output. Will do that when I touch on that PR again.

php.ini-development Outdated Show resolved Hide resolved
@Girgias Girgias force-pushed the Girgias:PHP-7.4 branch from 61c9ad4 to 79a9776 Apr 24, 2019
@nikic
nikic approved these changes Apr 24, 2019
Copy link
Member

nikic left a comment

LGTM technically. I'll wait with merging this until the internals discussion settles.

@Girgias

This comment has been minimized.

Copy link
Contributor Author

Girgias commented Apr 24, 2019

LGTM technically. I'll wait with merging this until the internals discussion settles.

No problem, I mean if the default change is a concern I don't mind reverting that and only have the deprecation notice for PHP 7.4.

@jrfnl

This comment has been minimized.

Copy link
Contributor

jrfnl commented Jun 10, 2019

Just checking in: what's the status of this ?

@Girgias

This comment has been minimized.

Copy link
Contributor Author

Girgias commented Jun 10, 2019

Just checking in: what's the status of this ?

Well, I'll need to have a look at it again to change the implementation to go away from this controversial implementation and maybe even need to push it through the RFC process again.
So status: ¯_(ツ)_/¯

@jrfnl

This comment has been minimized.

Copy link
Contributor

jrfnl commented Jun 10, 2019

Thanks @Girgias for getting back to me. Would it be safe to presume that the chances of this making it into 7.4 are getting slim ?

@Girgias

This comment has been minimized.

Copy link
Contributor Author

Girgias commented Jun 10, 2019

Thanks @Girgias for getting back to me. Would it be safe to presume that the chances of this making it into 7.4 are getting slim ?

As is yes, will try to work on it tomorrow.

@nikic nikic added this to the PHP 7.4 milestone Jun 14, 2019
@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jun 14, 2019

Thanks @Girgias for getting back to me. Would it be safe to presume that the chances of this making it into 7.4 are getting slim ?

We're still a few weeks away from feature freeze. I think that most likely this will land, with the updated implementation approach.

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jun 17, 2019

Closing in favor of #4263.

@nikic nikic closed this Jun 17, 2019
@Girgias Girgias deleted the Girgias:PHP-7.4 branch Jun 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.