? ? Success

User tests: Successful: Unsuccessful:

avatar nonumber
nonumber
22 Jul 2015

This PR fixes some more class/method/function names to comply with the CamelCase rules mentioned here:
http://joomla.github.io/coding-standards/?coding-standards/chapters/php.md

...be written in CamelCase even if using traditionally uppercase acronyms (such as XML, HTML).

Also see: joomla/joomla-framework#377

avatar nonumber nonumber - open - 22 Jul 2015
avatar nonumber nonumber - change - 22 Jul 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 22 Jul 2015
Labels Added: ? ?
avatar Bakual
Bakual - comment - 22 Jul 2015

PS: Not sure how to commit case changes in file names.

What do you mean? Changing filename casing would be a bad idea. We just had an issue with that.

avatar mbabker
mbabker - comment - 22 Jul 2015

If you're referring to the unit tests don't worry about renaming files there right now.

avatar Bakual
Bakual - comment - 22 Jul 2015

Ah I see what you did. Travis catched it :smile:

Keep in mind that on Linux systems, filenames are case sensitive and thus they have to match exactly.
On Windows systems, that is another story, there it wouldn't matter.

Renaming the files is out of topic, since on Linux we would end up having two files, while on Windows we only have one.

avatar nonumber
nonumber - comment - 22 Jul 2015

Ok, so should I not commit the filename fixes in the test folder?

avatar mbabker
mbabker - comment - 22 Jul 2015

@Bakual The unit test classes don't get packaged, so the issue we hit with the loader will never occur for 99.9% of people. Like I mentioned above, there's really no need to rename the test files. If you're adamant about doing it though, what I usually do is something like this:

git mv path/to/OldFile.php Oldfile.php && git mv Oldfile.php path/to/Oldfile.php
avatar nonumber
nonumber - comment - 22 Jul 2015

Yeah, I found out how to change the filenames :)

avatar nonumber
nonumber - comment - 22 Jul 2015

This PR isn't about fixing autoloading so much. More about fixing code styling, So good to have the correct casing throughout, also in test suite.

avatar mbabker
mbabker - comment - 22 Jul 2015

So good to have the correct casing throughout, also in test suite.

Not gonna argue that at all. But because of the conventions used in some places in the CMS, there may be places where an acceptable class/method name doesn't align with the code style preferences (hence my comment on the SEOHtml thing), so anything testing on those behaviors should remain in place. Otherwise, as long as we aren't breaking things, I see no issue with these PRs.

avatar nonumber nonumber - change - 22 Jul 2015
The description was changed
avatar nonumber
nonumber - comment - 22 Jul 2015

Ok, great :)
So hence a little testing is required on this PR. Not sure where/how to test the ExampleViewSeoHtml class though...

avatar Bakual
Bakual - comment - 22 Jul 2015

The unit test classes don't get packaged, so the issue we hit with the loader will never occur for 99.9% of people.

True, files in the test folder should be fine to be renamed.
Just don't do it on files that get packed for production :smile:

avatar nonumber
nonumber - comment - 22 Jul 2015

Ok, all done and good now :)

avatar brianteeman brianteeman - change - 24 Jul 2015
Category Code style
avatar infograf768
infograf768 - comment - 26 Jul 2015

Why do we have here a Tag "Language Change" ? I see no ini modified or added.

avatar nonumber
nonumber - comment - 26 Jul 2015

I think because it touches files like administrator/language/en-GB/en-GB.localise.php

avatar brianteeman
brianteeman - comment - 26 Jul 2015

It's an automatic tag based on the folder

avatar mbabker
mbabker - comment - 26 Jul 2015

Ya, it was a purposeful thing to make the code that adds that label scan for all changes in the language folders versus just the INI files. For example, if something significant changes in the localise.php files or the XML files, for those who are following language changes in the repo and not waiting for the language freeze to be given a diff, they can also check those extra files to see if some change applies to them.

avatar infograf768
infograf768 - comment - 26 Jul 2015

OK, got it. Did not see the change in localise.php

-abstract class En_GBLocalise
+abstract class En_GbLocalise

Will it be necessary to change all our xx-XX.localise.php or is this B/C?

avatar mbabker
mbabker - comment - 26 Jul 2015

It's B/C. Since these classes aren't autoloaded, you could write it as En_gBlocALIse and PHP could care less. It's just a matter of code standards and consistency.

If they were autoloaded either using JLoader or Composer's autoloader, there'd be a little more strictness on class and file names matching a convention.

avatar infograf768
infograf768 - comment - 26 Jul 2015

Thanks for the explanation.

avatar nonumber
nonumber - comment - 10 Aug 2015

So is this good to go?

avatar Bakual
Bakual - comment - 10 Aug 2015

Someone has to review it. I haven't had the time yet. If someone can do it, pleae do :smile:

avatar nonumber
nonumber - comment - 26 Jan 2016

Been too long. Out of sync. So closing this.
Effort and time lost.

avatar nonumber nonumber - change - 26 Jan 2016
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2016-01-26 23:58:29
Closed_By nonumber
avatar nonumber nonumber - close - 26 Jan 2016
avatar nonumber nonumber - head_ref_deleted - 28 Jan 2016

Add a Comment

Login with GitHub to post a comment