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

Fix array offset notice on PHP 7.4 when installing extensions #29416

Merged
merged 2 commits into from
Jun 15, 2020
Merged

Fix array offset notice on PHP 7.4 when installing extensions #29416

merged 2 commits into from
Jun 15, 2020

Conversation

SharkyKZ
Copy link
Contributor

@SharkyKZ SharkyKZ commented Jun 4, 2020

Partial Pull Request for Issue #29414.

Summary of Changes

Fixes a notice on PHP 7.4 when uploading extension package fails.

Testing Instructions

  1. Install an extension package with size over upload_max_filesize limit. Check PHP error log.
  2. Try to install a random file, e.g. an image. Check Joomla's tmp directory.
  3. Install an extension with size under upload_max_filesize limit.

Expected result

  1. Installation aborted. No PHP notices.
  2. Installation aborted and uploaded file is removed from tmp directory.
  3. Extension installed successfully.

Actual result

  1. Installation aborted. PHP notice: Trying to access array offset on value of type bool in administrator\components\com_installer\models\install.php on line 178

Documentation Changes Required

No.

@richard67
Copy link
Member

@SharkyKZ For some reason I don't get the PHP notice of issue 1. with PHP 7.3. I get the correct red alert telling that maximum PHP file upload size is too small.

@SharkyKZ
Copy link
Contributor Author

SharkyKZ commented Jun 4, 2020

@richard67 The notice is not visible. You have to check PHP error log.

@richard67
Copy link
Member

@SharkyKZ I always check PHP error log and have error reporting set to maximum in the backend. In my php.ini file I have E_ALL & ~E_DEPRECATED & ~E_STRICT, and I have log_errors = On; in my php.ini and a path to a log file set, which I watch permanently when testing and which shows me all. But the notice mentioned here I did not see.

@richard67
Copy link
Member

@SharkyKZ Maybe it happens only with PHP 7.4?

@SharkyKZ
Copy link
Contributor Author

SharkyKZ commented Jun 4, 2020

@richard67 You're right. It's only on PHP 7.4+.

@SharkyKZ SharkyKZ changed the title Fix array offset notice when installing extensions Fix array offset notice on PHP 7.4 when installing extensions Jun 4, 2020
@richard67
Copy link
Member

@SharkyKZ Then I can't test it, sorry :-(
Code review looks good though.

@mbabker
Copy link
Contributor

mbabker commented Jun 4, 2020

Code change looks fine.

@@ -135,6 +135,14 @@ public function install()
return false;
}

// Check if package was uploaded successfully.
if (!\is_array($package))
Copy link
Member

Choose a reason for hiding this comment

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

I would remove here the \ to be consistent with the rest of the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whilst consistency is definitely largely good for totally selfish reasons this will make my life easier merging to J4 😅 so merging as it is

@laoneo
Copy link
Member

laoneo commented Jun 5, 2020

I have tested this item ✅ successfully on f850a21


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

1 similar comment
@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on f850a21


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

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 5, 2020
@wilsonge wilsonge merged commit fcccb1d into joomla:staging Jun 15, 2020
@wilsonge
Copy link
Contributor

Thanks!

@joomla-cms-bot joomla-cms-bot removed RTC This Pull Request is Ready To Commit labels Jun 15, 2020
@wilsonge wilsonge added this to the Joomla! 3.9.20 milestone Jun 15, 2020
@SharkyKZ SharkyKZ deleted the j3/notice/installer branch June 16, 2020 05:17
Reconix pushed a commit to Reconix/joomla-cms that referenced this pull request Aug 31, 2020
Reconix added a commit to Reconix/joomla-cms that referenced this pull request Aug 31, 2020
Reconix added a commit to Reconix/joomla-cms that referenced this pull request Aug 31, 2020
Reconix added a commit to Reconix/joomla-cms that referenced this pull request Aug 31, 2020
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.

7 participants