[#32282] Move joomla/form/fields to joomla/form/field for autoloading

[wilsonge]

This is the same PR #2234 but at 3.3 dev rather than master

http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=32282&start=0

[mbabker]

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 😉

[wilsonge]

OK so what do I need to do?

[mbabker]

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

[wilsonge]

Done :)

[betweenbrain]

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

[mbabker]

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.

[wilsonge]

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

[betweenbrain]

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

[wilsonge]

Yes. definitely!!

[betweenbrain]

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

I'll try with com_patchtester then

[wilsonge]

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

[wilsonge]

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 :)

[mbabker]

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

[mbabker]

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.

[mbabker]

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).

[mbabker]

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.

[betweenbrain]

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 GitHubhttps://github.com//pull/3116#issuecomment-39267632
.

[phproberto]

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.

[mbabker]

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.

[Bakual]

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.

[wilsonge]

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?

[mbabker]

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.

[phproberto]

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

[mbabker]

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.

[phproberto]

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.

[mbabker]

Ahh, gotcha!

[Bakual]

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

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