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

Replace curly braces for PHP 7.4 compatibility #25782

Merged
merged 8 commits into from Sep 28, 2019

Conversation

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Aug 5, 2019

Summary of Changes

Replaces curly brackets used for array and string access with square brackets to solve deprecation warnings in PHP 7.4:

Array and string offset access syntax with curly braces is deprecated

Testing Instructions

Code review?

Documentation Changes Required

No.

@SharkyKZ SharkyKZ requested review from rdeutz, wilsonge and zero-24 as code owners Aug 5, 2019
@SharkyKZ SharkyKZ changed the title Replace curly braces for PHP 7.4 compat Replace curly braces for PHP 7.4 compatibility Aug 5, 2019
@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Aug 5, 2019

Given this as a PR to @nikosdion https://github.com/akeeba/angie/pull/76 so we can try and get an official version of the restore.php code with this in. Rather than starting to monkey patch this ourselves. If we do need to patch this way we'll need to modify the copyright at the top of restore.php similar to how we did in fof with 4889f00

@SharkyKZ

This comment has been minimized.

Copy link
Contributor Author

SharkyKZ commented Aug 5, 2019

Should we make changes to FoF too? And what about other libraries like idna_convert and lessphp?

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Aug 5, 2019

Should we make changes to FoF too? And what about other libraries like idna_convert and lessphp?

Never change an upstream library. If needed submit a pull request to that libraries vendor

@SharkyKZ

This comment has been minimized.

Copy link
Contributor Author

SharkyKZ commented Aug 5, 2019

Those libraries aren't maintained. We did make changes to FoF in the past https://github.com/joomla/joomla-cms/commits/staging/libraries/fof.

@zero-24 zero-24 removed their request for review Aug 5, 2019
@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Aug 5, 2019

FOF is fine to change. idna convert is also fine as it's something internal to string and doesn't exist on packagist. i already did that one in the framework joomla-framework/string@33944ac

Let's try submitting lessphp back to them - leafo/lessphp@v0.5.0...master but there hasn't been a release in nearly 5 years - so unsure whether they'll actually accept it or not.

@SharkyKZ

This comment has been minimized.

Copy link
Contributor Author

SharkyKZ commented Aug 5, 2019

Submitted leafo/lessphp#647.

SharkyKZ added 2 commits Aug 5, 2019
FOF
@nikosdion

This comment has been minimized.

Copy link
Contributor

nikosdion commented Aug 6, 2019

Hey, guys. Thank you for the PR! George made it to the wrong repo but don't worry, I know where else to apply it :)

@wilsonge Do you want me to make a PR with the new version of restore.php? If so, only J3 or both J3 and J4?

@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Aug 6, 2019

Thankyou very much! Sorry for the wrong repo thing. Just to j3 please and it will get merged upto j4

@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Aug 6, 2019

@SharkyKZ can you remove the restore.php changes from this PR please and they'll be covered in @nikosdion 's separate PR

@SharkyKZ

This comment has been minimized.

Copy link
Contributor Author

SharkyKZ commented Aug 6, 2019

Done.

@nikosdion nikosdion referenced this pull request Aug 6, 2019
@nikosdion

This comment has been minimized.

Copy link
Contributor

nikosdion commented Aug 6, 2019

I PR'ed the new restore.php, see #25793

BTW, has anyone managed to reliably run PHP 7.4 betas on either macOS or Ubuntu 19.04? I don't want to have to pull in half of the known universe to compile it myself on Ubuntu, I really don't have the time anymore :/

@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Aug 7, 2019

I haven't yet - but we're doing it in drone with the php 7.4 nightly docker container - so i think that's where i'm going to start at https://github.com/joomla/joomla-cms/blob/4.0-dev/.drone.yml#L73

@nikosdion

This comment has been minimized.

Copy link
Contributor

nikosdion commented Aug 7, 2019

@wilsonge OK, that's pretty much what I thought I would have to do. I am not a huge fan of dockerizing everything because of various issues I've had with XDebug, hostnames, testing CLI applications and ownership / permissions of files in mounted volumes but I digress. I am now using https://github.com/devilbox/docker-php-fpm-7.4 because it solves the ownership / permissions issue and creates a tidy PHP-FPM server I can use with my localhost setup, e.g.:

docker run \
	--rm -d --name devilbox-php74 \
	-e NEW_UID=$(id -u) \
	-e NEW_GID=$(id -g) \
	-e TIMEZONE="Europe/Athens" \
	-e DOCKER_LOGS=0 \
	-e FORWARD_PORTS_TO_LOCALHOST="3306:host.docker.internal:3306, 1025:host.docker.internal:1025, 6379:host.docker.internal:6379" \
	-p 127.0.0.1:9074:9000 \
	-v $(pwd)/logs:/var/log/php \
	-v $(pwd)/php_config:/etc/php-custom.d \
	-v /path/to/my/sites:/path/to/my/sites \
	-v /path/to/my/git/repos:/path/to/my/git/repos \
	-t devilbox/php-fpm:7.4-work

to start the PHP-FPM server and then a couple of lines in my .htaccess to use it:

<FilesMatch "\.php$">
SetHandler "proxy:fcgi://127.0.0.1:9074/"
</FilesMatch>
@C-Lodder

This comment has been minimized.

Copy link
Member

C-Lodder commented Aug 9, 2019

R.E the lessphp library, it looks like one of the forks has a lot of PR ported from the original repo and has more interaction: https://github.com/MarcusSchwarz/lesserphp

May be an idea to submit your PR there

@SharkyKZ

This comment has been minimized.

Copy link
Contributor Author

SharkyKZ commented Aug 9, 2019

@C-Lodder I don't think that library is compatible with ours.

@rdeutz
rdeutz approved these changes Aug 15, 2019
@Quy

This comment has been minimized.

Copy link
Contributor

Quy commented Sep 27, 2019

I have tested this item successfully on 2861347


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25782.

@mbabker

This comment has been minimized.

Copy link
Member

mbabker commented Sep 27, 2019

Files in libraries/idna_convert need a notification they have been altered.

@SharkyKZ

This comment has been minimized.

Copy link
Contributor Author

SharkyKZ commented Sep 27, 2019

@mbabker Added note. Thanks.

@HLeithner HLeithner merged commit 3601da2 into joomla:staging Sep 28, 2019
4 checks passed
4 checks passed
Hound No violations found. Woof!
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr Build is passing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@HLeithner

This comment has been minimized.

Copy link
Member

HLeithner commented Sep 28, 2019

Thanks for fixing deprecated the syntax.

@HLeithner HLeithner added this to the Joomla! 3.9.13 milestone Sep 28, 2019
@SharkyKZ SharkyKZ deleted the SharkyKZ:j3/php74 branch Sep 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.