? Pending

User tests: Successful: Unsuccessful:

avatar SharkyKZ
SharkyKZ
4 Jul 2019

Pull Request for Issue # .

Summary of Changes

Rename SysinfoModel.php to SysInfoModel.php.
Typo in Joomla\Component\Privacy\Site\Service\Router.

Testing Instructions

Code review? CC @wilsonge

Documentation Changes Required

No.

avatar SharkyKZ SharkyKZ - open - 4 Jul 2019
avatar SharkyKZ SharkyKZ - change - 4 Jul 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 4 Jul 2019
Category Administration com_admin Front End
avatar SharkyKZ
SharkyKZ - comment - 4 Jul 2019

Don't know if changing file casing causes issues at this point. If it does, we can rename the class instead?

avatar brianteeman
brianteeman - comment - 4 Jul 2019

typo looks correct to me
not sure about the camelcase name change - renaming this way always causes issues

avatar richard67
richard67 - comment - 4 Jul 2019

Maybe just leave the file name as it is and only correct the typo?

avatar SharkyKZ
SharkyKZ - comment - 4 Jul 2019

@richard67 that won't fix the issue. We need to either rename the file or rename the class and change all instances of it in code.

Anyways, this is fine, I think, if updates between alphas aren't officially supported.

avatar richard67
richard67 - comment - 4 Jul 2019

@SharkyKZ Ah, yes I forgot that with the update support ;-)

avatar SharkyKZ
SharkyKZ - comment - 4 Jul 2019

Hm, there are more instances of this:

\Joomla\Component\Menus\Administrator\Field\ComponentsCategoryField
\Joomla\Component\Menus\Administrator\Field\MenuOrderingField

Rename classes or rename files? Renaming files also requires updating XML files but it would be nice to have correctly cased classes at some point.

avatar SharkyKZ SharkyKZ - change - 4 Jul 2019
Labels Added: ?
avatar richard67
richard67 - comment - 4 Jul 2019

Hmm, I don't remember right now how such renames were handled in past, was not involved in that, just reading some PR when passing by long time ago. George maybe remembers that better.

avatar wilsonge
wilsonge - comment - 4 Jul 2019

We had to rename them in two commits - to a different name and back again for git to be happy. Then that still doesn't work on shared hosts (but at this stage in j4 we don't guarentee b/c there so it's not a problem).

TL/DR - just rename the files and let's do this correctly and set the correct precedent :)

avatar joomla-cms-bot joomla-cms-bot - change - 5 Jul 2019
Category Administration com_admin Front End Administration com_admin com_menus Front End
avatar joomla-cms-bot joomla-cms-bot - change - 5 Jul 2019
Category Administration com_admin Front End com_menus Administration com_admin com_categories com_menus Front End
avatar SharkyKZ
SharkyKZ - comment - 5 Jul 2019

Hm, I think it's better to rename SysInfoModel and revert the filename change. The view is called Sysinfo so it tries to load SysinfoModel by default anyways.

avatar joomla-cms-bot joomla-cms-bot - change - 5 Jul 2019
Category Administration com_admin Front End com_menus com_categories Administration com_categories com_menus Front End
avatar joomla-cms-bot joomla-cms-bot - change - 5 Jul 2019
Category Administration Front End com_menus com_categories Administration com_admin com_categories com_menus Front End
avatar SharkyKZ
SharkyKZ - comment - 5 Jul 2019

Unless we want to rename the view, which also means updating all links to it.

avatar wilsonge
wilsonge - comment - 5 Jul 2019

No I think keeping the existing view type makes sense

avatar SharkyKZ
SharkyKZ - comment - 6 Jul 2019

So this PR should be fine as it is. Just found more of the same:

\Joomla\CMS\Encrypt\AES\Openssl
\Joomla\CMS\Filesystem\Support\StringController
\Joomla\CMS\Form\Field\SQLField

StringController has already been reported for 3.x (#23725). Other two are 4.0-specific.

avatar joomla-cms-bot joomla-cms-bot - change - 6 Jul 2019
Category Administration Front End com_menus com_categories com_admin Administration com_admin com_categories com_menus Front End Libraries
avatar wilsonge wilsonge - change - 10 Jul 2019
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2019-07-10 09:33:21
Closed_By wilsonge
avatar wilsonge wilsonge - close - 10 Jul 2019
avatar wilsonge wilsonge - merge - 10 Jul 2019
avatar wilsonge
wilsonge - comment - 10 Jul 2019

This will do as a first pass - thankyou @SharkyKZ !

avatar Orgoth
Orgoth - comment - 11 Jul 2019

Do these changes also work under Windows?
In my ticket (#23725) the developers also tried to rename the StringController and encountered problems with the update mechanism when changing a file name to upper or lower case under Windows.

Quote #23762:

I have tested this item unsuccessfully on a0538dc

In windows this PR breaks the whole deletion process, because realpath converts all slashes to \, so it's always not true (and the JFile::delete will never be called).

Additional there will not be 2 files in windows, so the deletion process (if it works) would delete the whole file without replacement.

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

avatar SharkyKZ
SharkyKZ - comment - 11 Jul 2019

@Orgoth this PR fixes only new files added in 4.0. Updates between 4.0 dev builds aren't supported, so if you try to update from older 4.0 version, you'll have to apply the filename changes manually or just use a new 4.0 installation.

This, however, won't be an issue when updating from 3.x because these files do not exist in 3.x.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 11 Jul 2019

Updates from Nightly > Nightly Build will be supported at Beta.

avatar wilsonge
wilsonge - comment - 12 Jul 2019

@SharkyKZ The field class rename broke the system tests so we've reverted back to how it was for now #25530 and will need to come up with an alternative fix

avatar SharkyKZ
SharkyKZ - comment - 12 Jul 2019

@wilsonge My bad, it's a field class ?‍♂ . We'd need to update the type in forms to use the new casing. As a simple alternative we could rename the class to SqlField.

avatar wilsonge
wilsonge - comment - 12 Jul 2019

Lets just rename the class back to lowercase I think - i mean uppercase SQL is just a nice to have

avatar wilsonge
wilsonge - comment - 12 Jul 2019

The casing in the field type hasn't changed from 3.x so I'd rather not change it

Add a Comment

Login with GitHub to post a comment