Failure

User tests: Successful: Unsuccessful:

avatar wilsonge wilsonge - open - 15 Feb 2014
avatar mbabker
mbabker - comment - 26 Mar 2014

Just for reference, the last Travis build actually passed on here. Don't know why (maybe it has to do with merging from master instead of staging, or even 3.3-dev), but it got flagged by my script as aimed to master.

With that said, the inability to automatically merge this isn't from my script :wink:

avatar wilsonge
wilsonge - comment - 26 Mar 2014

OK so what do I need to do?

avatar mbabker
mbabker - comment - 26 Mar 2014

Merge from 3.3-dev as that is the branch you've targeted with this PR.

avatar wilsonge wilsonge - change - 26 Mar 2014
Title
Move joomla/form/fields to joomla/form/field for autoloading
[#32282] Move joomla/form/fields to joomla/form/field for autoloading
avatar wilsonge wilsonge - change - 26 Mar 2014
Title
Move joomla/form/fields to joomla/form/field for autoloading
[#32282] Move joomla/form/fields to joomla/form/field for autoloading
avatar wilsonge
wilsonge - comment - 26 Mar 2014

Done :)

avatar betweenbrain
betweenbrain - comment - 1 Apr 2014

I hope I'm not being daft, but this PR seems to be renaming libraries/joomla/form/fields/accesslevel.php to libraries/joomla/form/field/accesslevel.php while still calling libraries/joomla/form/fields/accesslevel.php in administrator/components/com_admin/script.php. When I apply the diff, libraries/joomla/form/fields/predefinedlist.php is the only file left at libraries/joomla/form/fields/ and libraries/joomla/form/field/ does not exist. With it applied, I get errors like Fatal error: Class 'JFormFieldList' not found in /libraries/cms/form/field/menu.php on line 25

avatar mbabker
mbabker - comment - 1 Apr 2014

In com_admin/script.php, we have to delete old files out of the system; that's what's happening in that part of the PR.

avatar wilsonge
wilsonge - comment - 1 Apr 2014

Looks like predefined list got added after I made this PR - so just committing a fix to move that across. I have no clue why the patch isn't creating the new files tho! It definitely should

avatar betweenbrain
betweenbrain - comment - 1 Apr 2014

Am I correct in understanding that, for example libraries/joomla/form/field/accesslevel.php, should exist with the diff applied?

avatar wilsonge
wilsonge - comment - 1 Apr 2014

Yes. definitely!!

avatar betweenbrain
betweenbrain - comment - 1 Apr 2014

I have no clue why the patch isn't creating the new files tho! It definitely should

I'll try with com_patchtester then

avatar wilsonge
wilsonge - comment - 1 Apr 2014

OK and I'll merge in upstream again just in case........

avatar wilsonge
wilsonge - comment - 1 Apr 2014

OK there was a conflict in script.php from the JRegistry merges last night. It might have been that screwing up the patch? Anyhow try again :)

avatar mbabker
mbabker - comment - 1 Apr 2014

Things look good from here at a quick run through the CMS.

avatar mbabker
mbabker - comment - 1 Apr 2014

OK, so I've got an odd case. Apply the patch and run the install app. It fails for me with a "Could not connect to MySQL" message on building the language field on the first page.

avatar mbabker
mbabker - comment - 1 Apr 2014

OK, found the issue. We need to rename the language field in the install app. With the autoloader changes, there's now two JFormFieldLanguage classes; libraries/joomla/form/field/language.php (which is getting picked up) and installation/model/fields/language.php (which is the one we want).

avatar mbabker
mbabker - comment - 1 Apr 2014

This actually exposes an issue. Theoretically, a developer could override a field using this same type of logic in their extension; now they can't without overriding the autoloader. That's changes my thinking some.

avatar betweenbrain
betweenbrain - comment - 1 Apr 2014

I seem to recall reading on list about some devs doing this.

Best,

Matt Thomas
@betweenbrain
http://matt-thomas.me/
http://betweenbrain.com/
https://github.com/betweenbrain

Sent from mobile. Please pardon any typos or brevity.
On Apr 1, 2014 6:16 PM, "Michael Babker" notifications@github.com wrote:

This actually exposes an issue. Theoretically, a developer could override
a field using this same type of logic in their extension; now they can't
without overriding the autoloader. That's changes my thinking some.


Reply to this email directly or view it on GitHub#3116 (comment)
.

avatar phproberto
phproberto - comment - 1 Apr 2014

IMO that field override method should not exist. You will get unstable behavior on your extension if you use exactly the same class name.

Field should be named JFormFieldInstallLanguage or as stated by @mbabker installation can use a different prefix.

Anyway I don't see it as a B/C. It's a tricky way that shouldn't block the desired behavior which is to use autloading.

avatar mbabker
mbabker - comment - 1 Apr 2014

Agree or not, that's how the JForm API has been since 1.6 and a change now is arguably a B/C break (we have to change our own code to accommodate, that should say something). I agree it shouldn't work like it does today, but fact is, this is the current expected behavior.

avatar Bakual
Bakual - comment - 2 Apr 2014

IMO that field override method should not exist. You will get unstable behavior on your extension if you use exactly the same class name.

I agree fully that it should not exist and it's bad practice to use the same class name for own fields. You should rename the custom one. But given that it exists and we even use it in core ourself, it's clearly a B/C issue and has to wait for the next major. As bad as it is.

avatar wilsonge
wilsonge - comment - 2 Apr 2014

OK perhaps it would be a good start now to rename the language form field in the installer and to document on our documentation in J4 we will be removing the option to do this?

avatar mbabker
mbabker - comment - 8 Apr 2014

Change the type to type="installation.language" and rename the class to InstallationFormFieldLanguage (B/C in the install app isn't required, don't worry about proxies or anything crazy like that). Truthfully, I'd also suggest moving moving it to installation/form/field/language.php so the class is properly autoloaded, but not moving it will still work based on JForm's internal loader.

avatar phproberto
phproberto - comment - 8 Apr 2014

Why fix the installation field if the proposal is going to be rejected because it's suposed to break B/C?

avatar mbabker
mbabker - comment - 9 Apr 2014

The installation field proposal or this PR? This PR could be a 4.0 thing given it's B/C breaking nature as we had discussed.

In practice, we have freely made B/C breaking changes in the install app because of its isolated use case and not having a proper API (remember it was at 3.1 where we rewrote the full app onto the new MVC and JApplicationWeb, decoupling completely from "legacy" code). So fixing the install app's field wouldn't be an issue, and fixes our what we've decided to be incorrect use of the field.

avatar phproberto
phproberto - comment - 9 Apr 2014

I mean this PR can be closed. Maybe tag it with "v4.0"before closing to use it as reference?

Then if someone wants to create the field PR perfect.

avatar mbabker
mbabker - comment - 9 Apr 2014

Ahh, gotcha!

avatar Bakual Bakual - close - 9 Apr 2014
avatar Bakual
Bakual - comment - 9 Apr 2014

I mean this PR can be closed. Maybe tag it with "v4.0"before closing to use it as reference?

:+1:
Added a label "Reevaluate for v4.0" and closed this one. I think that makes indeed most sense to handle such issues.

avatar Bakual Bakual - change - 9 Apr 2014
Status New Closed
Closed_Date 0000-00-00 00:00:00 2014-04-09 06:41:42
Labels Added: ?
avatar Bakual Bakual - close - 9 Apr 2014
avatar zero-24 zero-24 - change - 24 Aug 2015
Milestone Added:
avatar zero-24 zero-24 - change - 24 Aug 2015
Milestone Added:

Add a Comment

Login with GitHub to post a comment