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

Stop extension installation if minimum requirement isn't met. #15890

Merged
merged 2 commits into from
May 29, 2017

Conversation

Bakual
Copy link
Contributor

@Bakual Bakual commented May 8, 2017

PR #3447 introduced a new class JInstallerScript which allows to specify a minimum PHP and Joomla version for extensions by simply setting the respective property in the extensions script file (assuming it extends that class).
The check is done during the preflight stage.

Summary of Changes

  • Changing those checks so they actually return false if the minimum requirement fails, aborting the installation with an error message. Currently it just shows the warning but the extensions still installs.
  • Changing the version check so it is indeed a minimum requirement. Currently the specified minimum version is considered "to low".

Testing Instructions

If you have an extension already using those features, test that it works as expected. Adjust the minimum versions and check that installation succeeds/fails as expected.

Expected result

If the installation doesn't meet the minimum requirements, the extension should not install.
If the installation meets exactly the minimum requirements, the extension should install without any message.

Actual result

Installation isn't aborted at all and if the minimum requirement is met exactly, the message is still shown.

Documentation Changes Required

None

Pinging @wilsonge since he was the original writer of those lines.

The specified minimum version is a valid version.
@Bakual Bakual changed the title Stop installation if minimum requirement isn't met. Stop extension installation if minimum requirement isn't met. May 8, 2017
Copy link
Contributor

@wilsonge wilsonge left a comment

Choose a reason for hiding this comment

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

Looks fine to me. The return false thing is clearly a bug. As to whether the minimum version should accept the same version, I guess it should, but I think that's more just an opinion than anything else shrug

@brianteeman
Copy link
Contributor

@Bakual if you can provide an extension to use for testing it will get tested faster :)

@Bakual
Copy link
Contributor Author

Bakual commented May 29, 2017

Sure thing. I've adjusted my component so it requires Joomla 3.7.3 and PHP 7.1.0 as minimum.
Without this PR you will get a message that it doesn't meet the minimum requirements but it still installs. With the PR applied the installation will be aborted and you get the message that either the PHP version (if you're under PHP 7.1.0) or the Joomla version doesn't meet the requirements.
com_sermonspeaker.zip

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 864de80


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

1 similar comment
@Quy
Copy link
Contributor

Quy commented May 29, 2017

I have tested this item ✅ successfully on 864de80


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

@brianteeman
Copy link
Contributor

@Bakual see how quick that was :)

@wilsonge wilsonge merged commit 8ec4b68 into joomla:staging May 29, 2017
@wilsonge wilsonge added this to the Joomla 3.7.3 milestone May 29, 2017
@Bakual Bakual deleted the JInstallerScriptFixVersionChecks branch May 30, 2017 05:50
izharaazmi added a commit to izharaazmi/joomla-cms that referenced this pull request May 31, 2017
* staging: (1779 commits)
  Deprecate modules correction (joomla#16362)
  Removed unnecessary code in com_content (joomla#14628)
  Stop installation if minimum requirement isn't met. (joomla#15890)
  Deprecate parts of com_modules (joomla#16152)
  LICENSE.txt (joomla#16317)
  [a11y] [com_fields] icons in modals (joomla#15047)
  Load jQuery in associations edit layout (joomla#16240)
  HHVM was removed. Remove from travis conditionals (joomla#16281)
  Fix Stylesheet Mime type keeping b/c (joomla#16284)
  re-merge joomla#15068 and joomla#16256
  Add showon attribute to add mailto link parameter (joomla#16282)
  Fix notices on Contact form (joomla#16279)
  Updating dutch TinyMCE files
  Media upload form margin (joomla#16253)
  Changes to display atom feeds correctly (joomla#16105)
  admin mod_latest (joomla#16277)
  Add truncate class and implement in popular articles module (joomla#16257)
  Using the "Special:MyLanguage" tag for links pointing to docs.joomla.org (joomla#15858)
  Change email notification to use site from address (Fix joomla#9261) (joomla#13518)
  Fix double encode ampersand in contact select list value (joomla#16268)
  ...
Bakual pushed a commit to Bakual/SermonSpeaker that referenced this pull request Jun 1, 2017
Bakual pushed a commit to Bakual/SermonSpeaker that referenced this pull request Nov 21, 2017
Bakual pushed a commit to Bakual/SermonSpeaker that referenced this pull request Nov 24, 2017
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.

6 participants