@wilsonge Maybe the security check which is enabled and corrected by this PR should be moved from DatabaseModel.php, routine initialise, to file SetupModel.php, routine validateDbConnection? For installation using an existing database there will not be a difference as far as i could observe, but how is it with installation including creation of a new database? Have no idea how that should work, never worked for me.

Still don't understand why there are new strings

Maybe check description and testing instructions then it is more clear?

@brianteeman I've changed it back. If the user has to created the file manually, then the created file is enough check that the user has access. It does not need to make him delete it in this case. Will change testing instructions in a minute.

1. Expected Result

That's not an error its an info message

2. Could do with some text to say what to do after deleting the text file. Because of the changes in the installer in J4 its not as obvious anymore

3. After deleting the file and clicking "Install Joomla" I get the spinning logo for a while and then I get
   

BUT checking the remote database shows that the tables etc have been created

@brianteeman

1. Expected Result
   That's not an error its an info message

You mean both warning and error? Or only the error one, and the warning is ok?
From an error handling point of view, in my opinion everything which stops installation from proceeding should be an error. But you are right, from an UX point of view it might be confusing.

I suggest we collect more feedback and if people agree change it then. There is nothing easier than just changing the message type. This also can also be done later, so message type discussion should not stop this PR.

2. Could do with some text to say what to do after deleting the text file. Because of the changes in the installer in J4 its not as obvious anymore

You mean to change the text in the alert of type error so they tell to click again the "Install Joomla" button after having deleted the file, and in case if the file had to be created manually click again the "Install Joomla" button after having created the file?

Please confirm if my understanding is right, or correct if not, and optionally propose the texts because for me as not native English speaker it is sometimes not easy to give enough information in as short as possible text.

When I'm sure that I've fully understood what you suggest, I'll implement the changes soon.

3. After deleting the file and clicking "Install Joomla" I get the spinning logo for a while and then I get ...

Can you reproduce this and reproduce that it doesn't happen without this PR? I had this one time when testing, and it seemed to me that just the database was a bit slow at this time for some unknown reason.

So I'm not sure if it is related to my PR, but if it turns out that it is related, let me know and I'll propose an alternative (moving the checks from databae model to setup model) in another branch which could be tested for this issue.

Meanwhile I'll click the "Update branch" button to see if drone fails again.

@brianteeman Maybe for your 2nd point:

To confirm that you are the owner of this website please leave this installation form open as it is and delete the file named \"%1$s\" we have created in the \"%2$s\" folder of your Joomla site. Then click the \"%3$s\" button again to continue with installation.

And for the other case:

We were not able to create the file. Please leave this installation form open as it is, manually create a file named \"%1$s\" and upload it to the \"%2$s\" folder of your Joomla site. Then click the \"%3$s\" button again to continue with installation.

("%3$s" will be the language text for the "Install Joomla" button, i.e. "Install Joomla" if English.

Is that ok, or is it too long, or is it bad English at some place?

Hmm, done again failing ... that's more often than I got used to ... maybe there is really something wrong with this PR and some DB timeout, like @brianteeman has observed during his tests?

@HLeithner Can it be that the mysql system tests fail because there is soething else than "localhost" or "127.0.0.1" or "::1" used as database host, and this magic environment variable which allows to bypass the security check is not set, so the test waits for someone to delete the file?

Or is "localhost" or "127.0.0.1" or "::1" used in our testing environment as database host? Then the security check will not be done and this is not the problem.

Do you know who knows that?

@brianteeman Regarding your 3rd point: If you can replicate that a few times with this PR, could you check if it is the same with PR #27371 ? It does the same as this PR here, just at another place in the code, functionality and testing is the same as here.

@brianteeman Regarding your points 1 and 2: Did you have something like this in mind?

My worry is that people put more attention on the warning than on the info, then they follow the link to the doc and read while their session for installation times out ;-)

Maybe we should bring both texts together to the info message, i.e. not show the warning but add its text to the bottom of the info message?

Please give feedback if you have time, it really is appreciated even if I don't always agree 100 %.

P.S.: Maybe in the second screen shot in the info box, the 1st sentence "We were not able to create the file." should be extended to "We were not able to create the file for website ownership verification." or "We were not able to create the file for the security check.", then it would be more clear. What do you think?

The remote installation failed again without this PR so maybe thts my connection and not related to this PR

It is not a notice it is information
keepalive is running on the installer so the session should not time out
simple rule about messages is the more text you write the less that people will read it.

@brianteeman Thanks for the info. I'll make a commit soon to avoid PHP warnings when the file can't be written, this will also speed the test up a bit in this case.

And then let's improve UI: Would be happy if you could check my above proposals and give feedback and maybe make better suggestions.

It is not a notice it is information
keepalive is running on the installer so the session should not time out
simple rule about messages is the more text you write the less that people will read it.

@brianteeman Ok, thanks for feedback, will try it here with information. Regarding length of text: You mean my extensions to the texts so they explain what to be done as next are too long? I thought that this was that what you had in mind with your 2nd point, to explain the next step.

It is not a notice it is information

@brianteeman I didn't find a predefined alert type "info" or "information", but I found "message", which is green. Was that what you had in mind?

<div class="alert alert-info">

It is almost clear now that system tests are failing in drone because they use a host name which requires the security check to run, and the environment variable to bypass that is not set.

I more and more think that this exactly has been the reason why this security check has been switched off for J4 by commenting out the code.

INSTL_DATABASE_HOST_IS_NOT_LOCALHOST_CREATE_FILE="We were not able to create the file. Please manually create a file named \"%1$s\" and upload it to the \"%2$s\" folder of your Joomla site. Then select \"Install Joomla\" to continue. "
INSTL_DATABASE_HOST_IS_NOT_LOCALHOST_DELETE_FILE="To confirm that you are the owner of this website please delete the file named \"%1$s\" we have created in the \"%2$s\" folder of your Joomla site. Then select \"Install Joomla\" to continue. ""
INSTL_DATABASE_HOST_IS_NOT_LOCALHOST_GENERAL_MESSAGE="You are trying to use a database host which is not on your local server. For security reasons, you need to verify the ownership of your web hosting account. <a href=\"%s\">Please read the documentation</a> for more information."

@brianteeman Shouldn't I make the "Install Joomla" be a parameter and use the translated language text for that? The button text might be translated.

hopefully translators are not daft and they will realise but then again from experience they are so a param is better

@brianteeman How can I get <div class="alert alert-info"> using Factory::getApplication()->enqueueMessage?

@brianteeman I see <div class="alert alert-info"> is something else:

To use these would be a change on how IU works here. Here I use Factory::getApplication()->enqueueMessage to enqueue alert messages, like all other validations in the installation do.

What else do you suggest? Shall I change the type of the "error" alerts to "Notice" (blue)? Or to "Message" (green)? These are the options we have with enqueueMessage beside "warning" and "error".

notice or message will be fine

On Sun, 29 Dec 2019 at 14:42, Richard Fath notifications@github.com wrote:

@brianteeman https://github.com/brianteeman I see

is something else:
[image: div-alert-info]
https://user-images.githubusercontent.com/7413183/71558275-9c3a3780-2a51-11ea-94d7-7bcd717fa070.png

To use these would be a change on how IU works here. Here I use
Factory::getApplication()->enqueueMessage to enqueue alert messages, like
all other validations in the installation do.

What else do you suggest? Shall I change the type of the "error" alerts to
"Notice" (blue)? Or to "Message" (green)? These are the options we have
with enqueueMessage beside "warning" and "error".

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/joomla/joomla-cms/pull/27360?email_source=notifications&email_token=AAJ4P4OQZC5UJ6PBCFCDLHTQ3CZO3A5CNFSM4KAPHR2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHZA6YQ#issuecomment-569511778,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAJ4P4O6IT627EFVUEUHEKDQ3CZO3ANCNFSM4KAPHR2A
.

--
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

@brianteeman Then I will use notice (blue), because message (green) is only used for success messages.

On the other side, green gets more focus from me than blue, compared to the yellow of the warning, so maybe green would be better for me.

But then I think of people having red-green color blindness, so at the end blue is better I think.

@brianteeman Is it ok now (see changed screenshots in description)? Or should we change the type of the warning to "notice", too?

I would also like to extend the text INSTL_DATABASE_HOST_IS_NOT_LOCALHOST_CREATE_FILE by the instruction to make the installation folder writable, or I have to revert my commit d1966dc , because it might confuse the user if he/she has created the file as requested but not changed folder permission so the check comes up with the same message again.

I know you don't like too long texts. But what is better? Extend it a bit so the user also corrects folder permissions, or change back the check in the code by reverting my commit so if the file is there installation goes on and succeeds, and only deleting the installation folder at the end might fail?

The last point I've decided to change it back to like it was, so language text is ok.

Drone system tests fixed with last commit for .drone.yml.

@brianteeman All your suggestions are implemented. I'd be happy if you could find time to test. And thanks for your time with the reviews up to now, I really appreciate that.

Drone passed ... is like bday and xmas and new year and easter at the same day ;-)

... ready for testing.

Seems all ok now but I cannot comment on the drone.yml changes

@brianteeman Yes, the drone is something for Robert, he was pinged for review. Thanks for all, would be nice if you could give it a good test on the issue tracker.

I signed your drone.yml

@HLeithner Thanks, I've forgotten that.

@zero-24 Please test if you can find some time.

@wilsonge or @HLeithner Should I add the release blocker label to this PR because the issue #18736 solved by it has that label? And should it be added to the "[4.0] Installer" project? Label I can add myself, but project not.

@Quy Thanks.

I have tested this item ✅ successfully on 6378972

i've simulated the "remote database host" via dcoker

I have tested this item ✅ successfully on 6378972

Works with r/w permission and with ro permissions.

Thanks a lot for reviews and testing. With 2 good tests it could be RTC, but there is a pending review request to @rdeutz as the code owner of the .drone.yml file, which is changed by this PR to define the environment variable necessary for skipping the security check in our testing environment for the system tests. Shall we wait for that review?

RTC

setting RTC doesn't hurt, maybe a way to have a more quick review

The only other way I know to overcome the security check is add additional code to joomla and that makes no sense for testing. So I'm merging this thanks all

Thanks.