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

[4.x] Fix installation JS for failed folder delete #37938

Merged

Conversation

richard67
Copy link
Member

@richard67 richard67 commented May 31, 2022

Pull Request for Issue #37909 (UX part) .

Summary of Changes

Let the installation JS disable fields which cannot be used anymore when there was an error with deleting the installation folder and make sure that the buttons to go to the admin or the site still work in this case.

Testing Instructions

Test 1 without the patch applied to reproduce the issue

This test should be made on Windows because on Linux file system permissions are fixed by the PHP code with "chmod", and Linux still can handle open file system handles of deleted files and folders, so it's hard to cause the error with deleting the installation folder.

  1. Make a new Joomla 4.1.x installation and proceed untill the last step where you can see the buttons to to go admin or site.

  2. If you can not reproduce issue Installation folder cannot be deleted on Windows #37909 or don't know if you can:
    Open a Windows Command Shell (CMD) Window and there change current directory to the installation folder, e.g. cd /d "D:\xampp\htdocs\joomla-41\installation".
    Leave that CMD Window open during the following steps.
    Alternatively you could set the permissions of some other file than the index.php, e.g. the "localise.xml" file, so that it cannot be modified or deleted anymore.
    The result would be the same in both cases:
    There is an error when deleting the installation folder, but the index.php in that folder and other necessary files to run the installer are deleted.

  3. Depends on if you are using a stable or a development version.

  • Stable version:
    Use one of the buttons to go to admin or site.
    Result: See 1st screenshot in section "Actual result BEFORE applying this Pull Request" below.
  • Development version (e.g. nightly build or local git clone):
    Use the button to delete the installation folder and then confirm in the confirmation dialogue.
    Result: See 2nd screenshot in section "Actual result BEFORE applying this Pull Request" below.
  1. Only if you are using a development version:
    Repeat step 3, i.e. use the button and confirm again.
    Result: See 3rd screenshot in section "Actual result BEFORE applying this Pull Request" below.

  2. Use the button to install additional languages, select some language(s) and use the "Next" button to proceed with installing the language(s).
    Result: 404 not found.

Test 2 with the patch applied to test the fix

This test should be made on Windows for the same reasons as stated above for test 1.

  1. Apply the patch of this PR. Since the JS of the installer is not handled by the build.js, the patch can be applied with Patchtester.

  2. Repeat steps 1 to 3 of the previous test "Test 1 without the patch applied to reproduce the issue".
    Result: Depends on if you are using a stable or a development version.

  • Stable version:
    See 1st screenshot in section "Expected result AFTER applying this Pull Request" below.
  • Development version (e.g. nightly build or local git clone):
    See 2nd screenshot in section "Expected result AFTER applying this Pull Request" below.
  1. Use one of the buttons to go to admin or site.
    Result: Button works.

  2. Repeat steps 2 and 3 but this time use the other button in step 3.
    Result: Button works.

Test 3 with the patch applied to test if everything else still works as before

This test should be made on an environment where you do not run into issue #37909 , i.e. not on Windows with PHP 8.1.

  1. Apply the patch of this PR. Since the JS of the installer is not handled by the build.js, the patch can be applied with Patchtester.

  2. Make a new Joomla 4.1.x installation and proceed untill the last step where you can see the buttons to to go admin or site.

  3. Use one of the buttons to go to admin or site.
    Result: Button works.

  4. Repeat steps 2 and 3 but this time use the other button in step 3.
    Result: Button works.

  5. Repeat step 2.

  6. Check everything works with installing additional languages, e.g. the "Skip" and the "Next" button both work.

Actual result BEFORE applying this Pull Request

Stable version

When using a stable version, the installation folder should be automatically deleted when using one of the 2 buttons to go to admin or site.

When deleting the folder has failed, you see the right error alert, and the 2 buttons buttons to go to admin or site are disabled.

The button to install additional languages is not disabled.

j41-installer-stable-before_1

You can start to install additional languages, but as soon as you have selected a language and use the "Next" button, you get a 404 error.

The only way out is to manually enter the right URL in the browser's URL field to go to the admin or site. If you don't know that you might think the installation is broken somehow.

Development version

When using a development version, there is an extra button to delete the installation folder.

When using that button and deleting the installation folder fails, you see the right error alert, and all buttons are still enabled.

j41-installer-development-before_1

When using the same button again, you see an error alert about fetching JSON data failed:

j41-installer-development-before_2

With installing languages you will have the same issue as described above for the stable version.

The only way out is to manually enter the right URL in the browser's URL field to go to the admin or site. If you don't know that you might think the installation is broken somehow.

Expected result AFTER applying this Pull Request

When deleting the installation folder has failed, the button to install additional languages is disabled, and the buttons to go to admin or site just are still enabled.

When using a development version, the button to delete the installation folder is also disabled in this case.

Stable version:
j41-installer-stable-after_1

Development version:
j41-installer-development-after_1

The buttons to go to admin or site work because they just redirect to the right place and not try to call any installation stuff again which does not exist anymore.

Documentation Changes Required

None.

@richard67
Copy link
Member Author

@brianteeman Should we extend the message in the warning alert by a hint that you can use the buttons to go to the admin or the site after having deleted the folder manually?

@richard67 richard67 requested a review from wilsonge May 31, 2022 11:56
@brianteeman
Copy link
Contributor

I dont think this is correct.

The error message could occur if the folder was not removed and it really wasnt empty. In that case you wouldnt want someone to be able to click on the two buttons to continue.

@richard67
Copy link
Member Author

I dont think this is correct.

The error message could occur if the folder was not removed and it really wasnt empty. In that case you wouldnt want someone to be able to click on the two buttons to continue.

As soon as something could have been removed, we do not really know if the installation folder is complete or not. So we can not trust in the installation still working, and the only way to continue are these 2 buttons which now will work.

The user has to delete the folder manually so or so, it doesn't make a difference.

@richard67
Copy link
Member Author

P.S.: Right now without the PR you already can go to the admin and site via the right URL without having deleted the installation folder manually after an error has happened with the automatic deletion. So this PR here does not really change that. It is still on the user to manually delete the folder in that case.

@chmst
Copy link
Contributor

chmst commented Jun 5, 2022

I have tested this item ✅ successfully on 8f08d13

Tested on Win11, xampp, php 8.1.5.

With Patch, the folder can be deleted and buttons for backend / Fronted are enabled.


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

@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on 8f08d13


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

@Quy
Copy link
Contributor

Quy commented Jun 16, 2022

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 16, 2022
@bembelimen bembelimen merged commit 39ac04b into joomla:4.1-dev Jun 18, 2022
@bembelimen
Copy link
Contributor

Thx

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 18, 2022
@bembelimen bembelimen added this to the Joomla 4.1.5 milestone Jun 18, 2022
@richard67 richard67 deleted the 4.1-dev-installer-cleanup-step-experiments branch June 18, 2022 10:00
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