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

restore.php update #25793

Closed
wants to merge 2 commits into from
Closed

restore.php update #25793

wants to merge 2 commits into from

Conversation

nikosdion
Copy link
Contributor

This commit syncs restore.php with the latest upstream development, including tentative PHP 7.4 support.

See gh-25782 for context

Summary of Changes

Updates com_joomlaupdate's restore.php. The main change you'll notice is tentative PHP 7.4 support. I call it tentative because at the time of this writing I do not have a working PHP 7.4 installation; I just followed the clues in the gh-25782 issue.

Testing Instructions

Run an update.

Documentation Changes Required

No impact.

This commit syncs restore.php with the latest upstream development,
including tentative PHP 7.4 support.
@@ -8325,7 +9243,7 @@ public function __toString()
}
if (function_exists('wincache_refresh_if_changed'))
{
wincache_refresh_if_changed(array($filename));
wincache_refresh_if_changed([$filename]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikosdion this square array syntax requires php 5.4. this is obviously a problem for the 5.3 support - i can't see anywhere else you're using it though - so is it accidental or is restore.php now php 5.4+?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've dropped PHP 5.3 support several months ago. Actually, I am not supporting anything older than PHP 5.6 since February. If you want to remove the square brackets you'll have to do it after applying this PR. I am not going back to supporting ancient PHP versions with a cumulative (that is, 5.3, 5.4 and 5.5 combined) market share under 10%. It makes no sense.

BTW: according to my stats, the only people really using PHP 5.3 are those stuck on ancient versions of Joomla. Use of PHP 5.3 in Joomla 3.9 is so low that it's not even statistically significant.

Copy link
Contributor

@wilsonge wilsonge Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine. I believe this is the only place so for now it's an ok tradeoff to fix a single line and obviously we don't care in J4 because we're supporting 7.2+.

So if it's ok with you we'll amend here - where would you like the copyright change. top of file or in this class or both?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top of the file please. Thank you!

@zero-24 zero-24 removed their request for review August 7, 2019 19:00
@HLeithner
Copy link
Member

I tried to update update from 3.9.2 to 3.9.11 with this restore.php but failed with "Invalid Login".

Also 3.9.11 to 3.9.11 doesn't work with the same error. Any idea?

@nikosdion
Copy link
Contributor Author

I need to update the JavaScript on com_joomlaupdate. I hadn't realised that the restore.php you had was truly ancient. However, there are two problems with that.

  1. There is no documentation on the code style for JavaScript. I already cancelled a planned contribution for J4 because I can't spend an infinite amount of time trying to guess the elusive coding standard or deal with the Hound's idiotic feedback which makes no sense.

  2. During the update restore.php would be overwritten. At this point all future requests would fail because the JS already in the browser is out of date. The only way to deal with that is have users install a separate update to Joomla Update BEFORE updating Joomla itself. This is something we had to do in the past for the same reason (when we added hardening to restore.php sometime around Joomla! 3.4 IIRC).

The latter issue is basically one of communication and you should be able to handle it. The first issue is, at the moment, a hard contribution block for me. If someone gives me instructions for automatically applying JS code style with phpStorm I can resume my contributions which require JavaScript changes. If you can't you either commit to accept my contributions despite JS code style errors OR you lose me as a contributor. Rule number 1 for keeping contributors is to not make them feel their time is not appreciated by the project.

@nikosdion
Copy link
Contributor Author

I think it's best if you just modify your current restore.php for PHP 7.4 compatibility and add a header notice. I'm not touching Joomla's JS code with a 30-foot pole seeing what kind of things its JS coding style wants me to do.

@nikosdion nikosdion closed this Aug 8, 2019
@HLeithner
Copy link
Member

Thank you for you try and I have the same opinion to make our restore.php php 7.4 compatible is a much safer and easier step then upgrading to your version.

@richard67
Copy link
Member

richard67 commented Aug 9, 2019

@HLeithner What about @nikosdion 's request for some JS code style docs? Do you know where to address that issue? Or could we link to some Hound docs in our docs? I don't want this to be ignored because don't want to block him and others from contributing. We currently have a lack of JS experts - I am pretty sure not one.

Update: Ah, I see it's about PHP code style. But that doesn't change that we should not block people from contributing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants