Installation cleanup #35168
Installation cleanup #35168
Conversation
wilsonge
force-pushed
the
installation-cleanup
branch
from
66eb8a0 to
cbdd6d0
Compare
|
I have tested this item ✅ successfully on cbdd6d0 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35168. |
|
@wilsonge System tests are failing because they expect the button to be shown. It needs to adapt them to the changes here. |
Co-authored-by: Brian Teeman <[email protected]>
Co-authored-by: Phil E. Taylor <[email protected]>
Co-authored-by: Brian Teeman <[email protected]>
Co-authored-by: Brian Teeman <[email protected]>
| @@ -38,6 41,12 @@ public function deleteInstallationFolder() | |||
| $return = File::move(JPATH_ROOT . '/robots.txt.dist', JPATH_ROOT . '/robots.txt'); | |||
| } | |||
|
|
|||
| if (function_exists('opcache_reset')) { | |||
| \opcache_reset(); | |||
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
This doesn't work for our case. we've deleted the folder not a single file. I agree we do need to get #32915 in though.
There was a problem hiding this comment.
In theory we can try and just invalidate installation/index.php as that's what we actually test for? But not sure whether it's really a lesser evil.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
OK So concretely @HLeithner on his server gets race conditions without this line. And opcache invalidating that line also doesn't seem to solve it. Unclear right now if opcache just takes a bit longer so solves the race condition or there's some concrete cache being solved. But we'll go with this for 4.0.0 and then we can spend some time and figure out if we're just masking the issue or not for 4.0.1. Again fully agree that we need #32915 - just it doesn't apply to this specific case.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
In theory we can try and just invalidate installation/index.php as that's what we actually test for? But not sure whether it's really a lesser evil.
Can someone make a new PR to change it that way?
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
@PhilETaylor I changed it now to invalidate cd2ea50
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
We'll fix drone this evening once work is done. |
This comment was marked as abuse.
This comment was marked as abuse.
|
@wilsonge I think @PhilETaylor is right here. Should he make a PR to change from "opcache_reset" to something like here for the "installation/index.php" file? |
Summary of Changes
Completing installation now deletes the installation directory "automatically" before continuing when not in development mode. When in development mode you continue to get the existing button.
This allows the user to delete installation rather than currently being stuck as the red button is already currently only shown in development mode. Massive shoutout to @zero-24 for last minute finding this one. Would have made for a very fast 4.0.1 otherwise :)
Testing Instructions
Actual result BEFORE applying this Pull Request
You can't delete the installation directory when the build is "stable"
Expected result AFTER applying this Pull Request
In development mode the installation directory is not deleted when clicking complete. Only when pressing the big red button. In stable mode it is only when you click either continue button the installation directory is deleted and you are allowed to proceed
Joomla\Version\Version::DEV_STATUSto beStable. Ensure you can't see the red button but when clicking on complete the installation directory is removed before you are allowed to proceedDocumentation Changes Required
None.