[com_fields] Move custom field types to plugin by laoneo · Pull Request #13319 · joomla/joomla-cms

laoneo · 2016-12-21T15:10:50Z

Pull Request for Issue #13227. It moves all the custom field types to their own plugin.

Summary of Changes

The text custom field type is moved to it's plugin. The JFormdomfieldinterface is removed.

The available field types are gathered trough the plugin event onCustomFieldsGetTypes.
The field value is prepared trough the onCustomFieldsPrepareFieldevent.
The dom is created trough the event onCustomFieldsPrepareDom.

If you are loading this PR trough the patch tester you need to discover the plugins first in the extension manager.

Testing Instructions 1

Create a new article custom field.
Create a new text field.
Go to Extensions -> Plugins -> Fields and disable the Text plugin.

Expected Result 1

The type box should only show Text and Gallery as options when creating a field.
The field should show up on the form and on the front of the article.

Testing Instructions 2

Go to Extensions -> Plugins -> Fields.
Disable the Text plugin.

Expected Result 2

The field should not be shown in the edit form and on the front end.

Documentation Changes Required

None, as there is no documentation already for custom fields.

Bakual · 2016-12-22T11:06:30Z

I have tested this item ✅ successfully on 47f11c0

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13319.}

Bakual · 2016-12-22T11:13:37Z

administrator/components/com_fields/libraries/fieldsplugin.php

+		$path = JPATH_PLUGINS . '/' . $this->_type . '/' . $this->_name . '/layouts';
+
+		// Prepare the value from the type layout
+		return JLayoutHelper::render('field.prepare.' . $field->type, array('field' => $field), $path);


I think having a folder structure plugins/fields/foo/layouts/fields/prepare/foo.php is a bit to much.
Do we really need the prefix field.prepare. for the layout path? I'd say field is a given anyway since it's located in the plugins/fields/foo/layouts directory. The prepare is likely superfluous as well.
Without that it would come down to plugins/fields/foo/layouts/foo.php.
I'd also say the layout name could be just default.php because in most cases we expect only one layout per plugin anyway. If there are multiple, it needs custom code anyway.

I would put it into a folder, otherwise the layout override in the template would be /templates/protostar/html/layouts/foo.php which is not optimal.

Maybe instead of using JLayoutHelper::render() we can use JPluginHelper::getLayoutPath() here? Then the layouts would go into the tmpl folder instead.
Similar to how it is done for the vote and pagenavigation plugin.
Then the override path would be html/plg_fields_text/default.php

Didn't know this exists, that's much better.

@Bakual although is possible to get the layout in the plugin html/plg_fields_text/default.php personally I would prefer the layouts to be in the root/layouts folder under joomla/custom_fields
e.g. layous/joomla/custom_fields/text.php

Two things about the /layouts folder:

The layouts folder should only be used for reusable JLayouts. I don't think that0s the case for most of them.

The layouts folder can't be used by 3rd party extensions without specific code in your install script that moves the files there. So it would give a wrong example.

Bakual · 2016-12-22T11:16:40Z

administrator/components/com_fields/libraries/fieldsplugin.php

+		// Gather the type
+		$type = $form->getValue('type');
+
+		if (isset($formData->type) && $formData->type)


Personal oppinion: Using if (!empty($formData->type) would do the exact same thing and is easier to read (once you get used to the "double negation" 😄 ).

If the the attribute type would not exist on $formData, a notice would be thrown.

Nah, empty() has an implied isset() check and doesn't throw a notice if the property doesn't exist. It does exactly what you did in two statements.
See http://php.net/manual/en/function.empty.php

Determine whether a variable is considered to be empty. A variable is considered empty if it does not exist or if its value equals FALSE. empty() does not generate a warning if the variable does not exist.

Didn't know that!

Bakual · 2016-12-22T11:20:41Z

I very much like this PR. It makes fetching the fields so much easier to understand. Thank you! 👍

laoneo · 2016-12-22T12:35:11Z

Should we put all fields into this PR, or should I do for every field a new one? The installer script needs then to be adjusted for every PR then, which will cause conflicts in other PR's which do change the installer sql files.

Bakual · 2016-12-22T12:47:04Z

I'd do all types in this PR. There will be no conflicts in the new files anyway and the other file changes are already in this PR.

This PR needs the install and update SQLs to register the added plugins and its default parameter values then.

infograf768 · 2016-12-22T13:43:38Z

please also, do not forget to modify en-GB install.xml for any new plugin

…la-cms.git into fields-text-plugin

laoneo · 2016-12-23T14:53:33Z

@Bakual what do you think about the latest commit? Stuff is moved to layout as the other plugins and the field parameters are inherited now from the plugin. If there are no more concerns, I will start to move the other ones.

zero-24 · 2016-12-23T14:56:54Z

administrator/components/com_fields/libraries/fieldsplugin.php

+		}
+
+		// Ensure it is an object
+		$formData = (object)$data;


Travis is complaining about a missing space $formData = (object) $data;

zero-24 · 2016-12-23T14:57:09Z

administrator/components/com_fields/libraries/fieldsplugin.php

+		// Gather the type
+		$type = $form->getValue('type');
+
+		if(!empty($formData->type))


and herer too :P

infograf768 · 2016-12-27T07:56:03Z

A few remarks concerning lang strings:

They should be alpha ordered
Both ini and sys.ini should contain for each plugin the two following constants/strings (here for Text)

PLG_FIELDS_TEXT="Fields - Text"
PLG_FIELDS_TEXT_XML_DESCRIPTION="The Text Fields plugin."

and for gallery

PLG_FIELDS_GALLERY="Fields - Gallery"
PLG_FIELDS_GALLERY_XML_DESCRIPTION="The Gallery Fields plugin."

The description should explain at minimum what it does... For example something like:
PLG_FIELDS_TEXT_XML_DESCRIPTION="This plugin lets create new fields of type 'Text" in the extensions where custom fields are implemented." //Correct my English...
No need to add a comment like ; Params and/or empty lines
No need to make the constant more complex than necessary by using for example:

PLG_FIELDS_TEXT_FIELDPARAMS_USE_GLOBAL="Use From Plugin"

instead of

PLG_FIELDS_TEXT_PARAMS_USE_GLOBAL="Use From Plugin"

https://laoneo@github.com/Digital-Peak/joomla-cms.git into fields-text-plugin Conflicts: administrator/components/com_fields/helpers/fields.php administrator/components/com_fields/helpers/internal.php administrator/components/com_fields/models/field.php administrator/components/com_fields/models/fields/type.php libraries/joomla/form/field.php plugins/fields/gallery/layouts/field/prepare/gallery.php

laoneo · 2017-01-06T09:30:07Z

Like 368042d ?

infograf768 · 2017-01-06T09:42:19Z

Concerning alpha order, it now looks fine. Thanks.

Question:
Any reason for adding JFormHelper::loadFieldClass('list'); to contentlanguage.php or frontend_language.php (these are just examples).

infograf768 · 2017-01-06T09:43:49Z

Oh, forgot
en-GB.lib_joomla.ini should also be patched in frontend. It should be the exact copy of the admin one.

waader · 2017-01-06T12:01:44Z

There is no text for PLG_FIELDS_GALLERY_XML_DESCRIPTION.

waader · 2017-01-06T12:35:36Z

Is it possible to show the configuration values set in the fields and system plugin?
Adding a text field for example, there is the field "filter" with the value "Use from plugin" or the field "Automatic Display" with the value "Use Global" and it would be nice to know what these values are without having to look them up.

Bakual · 2017-01-06T13:09:42Z

There is already an issue open for the global value: #13350

laoneo · 2017-01-07T08:41:08Z

Frontend lib file is changed.

Any reason for adding JFormHelper::loadFieldClass('list'); to contentlanguage.php or frontend_language.php

Because the JFormFieldList is not loaded automatically so I'v added the import to all fields which are extending list as I converted them back from abstractlist.

laoneo · 2017-01-07T08:44:17Z

Gallery string is created, thanks for the hint @waader.

Bakual · 2017-01-09T12:26:16Z

Finally found some time to test this.
Main result: It works, but there are a few bugs in the actual plugins.

@rdeutz Imho it could be merged to get the architecture right for the alpha2 and then fix the remaining bugs afterwards.

As for the findings in detail:

If a plugin is disabled, the created fields of that type still show as published. They should ideally show similar to modules where the actual extension is disabled. This can be done afterwards in a separate PR for sure as it's only a visual thing.
Checkboxes and Radio Field: "Checkboxes Values" and "Radio Values" respectively have no effect when set in the plugin parameters. They have to be set in the field parameters. Also, if no options are set a warning "Warning: DOMElement::setAttribute() expects parameter 2 to be string, object given in ...\administrator\components\com_fields\libraries\fieldsplugin.php on line 177" appears. This likely should be catched.
Calendar Field: The "Format" parameter set in the plugin has no effect because there is a default value also set in the field parameter.
Editor Field: Width and Height only apply to the textarea field when the editor is disabled. the editor itself always uses full width. Also the show button and hide button doesn't seem to have any effect.
List Field: For some reason that type doesn't show up in my list of available types.
Imagelist Field: Path settings don't work in Windows. Probably same issue we had with the Gallery type.
URL Field: Schemes parameter only work when explicitely set in the field, the one set in the plugin isn't considered.
User Field: "Multiple" doesn't work for that one and should be removed. The field doesn't support multiple selections.

laoneo · 2017-01-09T12:29:34Z

Thanks @Bakual. Give me some days to fix the bugs, except the first one.

Bakual · 2017-01-09T12:39:09Z

Imho those are all minor issues with the respective plugins and could be fixed after this is merged (to not delay this one). But I leave that for others to decide :)

Bakual · 2017-01-09T12:42:01Z

I have tested this item ✅ successfully on 2ccdcd7

Set as successful test after talking with @rdeutz

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13319.}

zero-24 · 2017-01-09T19:40:15Z

I have tested this item ✅ successfully on 2ccdcd7

looks good so far. I have just tested the plugin funktionality.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13319.}

zero-24 · 2017-01-09T19:40:49Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13319.}

* [plugins] - fields mssql installation plugins fields mssql installation #13616 * [com_fields] - mssql updates #13319 [com_fields] - mssql updates #13319 * remove duplicate extension id 462 for mssql removed id 462 to avoid duplicated id with #13626 cause now fields are managed with plugins Pull Request for Issue #13616 (comment)

laoneo added 10 commits December 21, 2016 10:40

Migrate text field to plugin

db151f8

Moved field description lookup to plugin event

b11a830

Remove not needed functions

dc3474d

Renamed to specification

38f39db

Adapt gallery

33bca35

Renamed to field types

ff07688

Remove dom interface

a51f325

Prepare the field trough an event

0db21ef

Remove domfield interface

1e2fd67

Add text layout

cb44627

joomla-cms-bot added Language Change This is for Translators PR-staging labels Dec 21, 2016

laoneo mentioned this pull request Dec 21, 2016

[com_fields] JFormAbstractlist should be named JFormFieldAbstractlist #13227

Closed

Bakual reviewed Dec 22, 2016

View reviewed changes

laoneo mentioned this pull request Dec 23, 2016

[com_fields] Register extension path for formfield lookup in com_fields #13300

Closed

laoneo changed the title ~~[com_fields] Fields text plugin~~ [com_fields] Move custom field types to plugin Dec 23, 2016

laoneo added 2 commits December 23, 2016 13:38

Merge branch 'staging' of https://laoneo@github.com/Digital-Peak/joom…

97fae0f

…la-cms.git into fields-text-plugin

Move output to layout and better multifields support

1116b01

laoneo force-pushed the fields-text-plugin branch from 47f11c0 to 1116b01 Compare December 23, 2016 14:51

zero-24 reviewed Dec 23, 2016

View reviewed changes

HoHoHoHo Travis

2d77dec

laoneo mentioned this pull request Dec 26, 2016

[com_fields] Can we drop fotorama plugin? #13375

Closed

Alpha ordering language strings

368042d

Adapting the frontend ini lib language file to be the same as back end

6e5843b

Add missing description language string for gallery

f245f39

laoneo mentioned this pull request Jan 7, 2017

[com_fields] MSSQL - sql for Custom fields #12660

Merged

zero-24 added the RTC This Pull Request is Ready To Commit label Jan 9, 2017

rdeutz merged commit 205d94b into joomla:staging Jan 9, 2017

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 9, 2017

laoneo deleted the fields-text-plugin branch January 9, 2017 20:40

laoneo mentioned this pull request Jan 17, 2017

[com_fields] MSSQL fieldplugins #13616

Closed

Uh oh!

Conversation

laoneo commented Dec 21, 2016 • edited by joomla-cms-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions 1

Expected Result 1

Testing Instructions 2

Expected Result 2

Documentation Changes Required

Uh oh!

Bakual commented Dec 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bakual commented Dec 22, 2016

Uh oh!

laoneo commented Dec 22, 2016

Uh oh!

Bakual commented Dec 22, 2016

Uh oh!

infograf768 commented Dec 22, 2016

Uh oh!

laoneo commented Dec 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infograf768 commented Dec 27, 2016

Uh oh!

laoneo commented Jan 6, 2017

Uh oh!

infograf768 commented Jan 6, 2017

Uh oh!

infograf768 commented Jan 6, 2017

Uh oh!

waader commented Jan 6, 2017

Uh oh!

waader commented Jan 6, 2017

Uh oh!

Bakual commented Jan 6, 2017

Uh oh!

laoneo commented Jan 7, 2017

Uh oh!

laoneo commented Jan 7, 2017

Uh oh!

Bakual commented Jan 9, 2017

Uh oh!

laoneo commented Jan 9, 2017

Uh oh!

Bakual commented Jan 9, 2017

Uh oh!

Bakual commented Jan 9, 2017

Uh oh!

zero-24 commented Jan 9, 2017

Uh oh!

zero-24 commented Jan 9, 2017

Uh oh!

Reviewers

Assignees

Labels

laoneo commented Dec 21, 2016 •

edited by joomla-cms-bot

Loading