Skip to content

[4.0] Update PrefixField.php in Installer#25144

Closed
Duke3D wants to merge 6 commits intojoomla:4.0-devfrom
Duke3D:patch-3
Closed

[4.0] Update PrefixField.php in Installer#25144
Duke3D wants to merge 6 commits intojoomla:4.0-devfrom
Duke3D:patch-3

Conversation

@Duke3D
Copy link
Contributor

@Duke3D Duke3D commented Jun 8, 2019

Pull Request for Issue # .
First report.

Summary of Changes

Added to validation of user-entered database prefix.

  1. user's response is converted to lowercase so that value stored in configuration.php is in alignment with the default behavior (convert to lowercase) of the MySQL database.
  2. test for and append underscore, if missing.

Testing Instructions

Run Installer.
A) At Database Prefix field, enter mixed or upper case prefix ending with underscore.
B) Repeat except enter mixed or upper case prefix without underscore.

Expected result

Installed database has lowercase prefix and value matches database prefix as stored in configuration.php.

Actual result

If not patched, installed database has lowercase prefix. Database prefix stored in configuration.php has mixed case prefix. Can lead to database issues on Linux servers and reference errors that appear on server migration.

Documentation Changes Required

None

When validating the user-entered database prefix, convert to lowercase so that value stored in configuration.php is in alignment with the convert to lowercase default behaviour of the MySQL database.
@infograf768
Copy link
Member

This deals only for the session prefix. It should also deal with new prefix when modified, therefore out of the if/else.
Also cs is bad.

@ghost ghost changed the title Update PrefixField.php in Installer [4.0] Update PrefixField.php in Installer Jun 9, 2019
@brianteeman
Copy link
Contributor

To satisfy the codestyle requirements please address the following issues

FILE: /********/src/installation/src/Form/Field/Installation/PrefixField.php
--
46 | --------------------------------------------------------------------------------
47 | FOUND 4 ERROR(S) AFFECTING 3 LINE(S)
48 | --------------------------------------------------------------------------------
49 | 82 \| ERROR \| Please consider a blank line preceding your comment
50 | 85 \| ERROR \| Please consider a blank line preceding your comment
51 | 86 \| ERROR \| Expected "if (...)\n"; found "if (...) "
52 | 86 \| ERROR \| Closing brace must be on a line by itself


@Quy
Copy link
Contributor

Quy commented Jun 21, 2019

@Duke3D Please do as requested here: #25144 (comment)

@ghost ghost added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Jun 25, 2019
@Schmidie64
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 0e7b5ca

After applying the patch, the prefix in the database is in lower case (without the patch it's also in lower case). In the configuration.php is the prefix still in mixed cases. So the pr doesen't work well.


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

@richard67
Copy link
Member

richard67 commented Jun 1, 2020

To fix #25144 (comment) , the checks have to be added to 2 places:

(Or maybe just the regex needs to be modified).

If my PR #29362 will be ready and merged, it will be only one place where to change that.

But I am in doubt if this PR here is right, because we had such check already for PostgreSQL, where it seems to matter, e.g. here:
https://github.com/joomla/joomla-cms/blob/4.0-dev/installation/src/Model/DatabaseModel.php#L133

So I think it could have had a reason why we only did it for PostgreSQL and not for MySQL.

But who knows, maybe it was wrong all the time for MySQL.

@roland-d
Copy link
Contributor

roland-d commented Aug 1, 2020

@Duke3D Any interest in picking this up or should the PR be closed?

@Quy
Copy link
Contributor

Quy commented Sep 5, 2020

Closing due to no response.

@Quy Quy closed this Sep 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Updates Requested Indicates that this pull request needs an update from the author and should not be tested.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants