[com_fields] Store fields in 'com_fields' property instead of reusing 'params' by Bakual · Pull Request #13353 · joomla/joomla-cms

Bakual · 2016-12-23T15:47:52Z

This decouples a part of #13182 since we don't seem to get an agreement there anytime soon 😄

Summary of Changes

Store fields in 'com_fields' property instead of reusing the already existing 'params'. This eliminates possible conflicts.
Show custom fields in frontend article edit using the joomla.edit.params JLayout.
Adjusted the JLayout to take a custom tab name so it is reusable better. If no tab name is specified it uses the current behavior ('myTab').

Testing Instructions

Test that you can still save values into custom fields both in frontend and backend.
Inspect the HTML code and see that the ID attribute of the field has changed from jform_params_alias to jform_com_fields_alias and the name from jform[params][alias] to jform[com_fields][alias].

Documentation Changes Required

None

ralain · 2016-12-23T23:00:41Z

I have tested this item ✅ successfully on c8ff6d1

(Assuming you mean jform[com_fields][alias])

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

Bakual · 2016-12-24T07:51:38Z

(Assuming you mean jform[com_fields][alias])

Of course! Adjusted the PR description now 😄

laoneo · 2016-12-24T08:42:11Z

I don't se the benefit for this change honestly? Params is made for custom fields, so why not reuse it?

Bakual · 2016-12-24T09:01:15Z

Params is made for custom fields, so why not reuse it?

Because it's not made for custom fields. It's a groupd specified in many (if not most) extensions XML file to hold various item options.
That means in most cases there is already data in that group, and it's the reason you load the existing data, convert it, merge with com_fields data, process it, unset it and convert the original params data back. All while having the risk that we have conflicts with existing data.
Now if using our own group, we can eliminate the risk of having conflicts and don't need to convert and merge and convert back the existing data.

laoneo · 2016-12-24T12:33:49Z

-		// Load the item params from the request
+		// Load the fields data from the request
 		$data = JFactory::getApplication()->input->post->get('jform', array(), 'array');



Fetching the data from the post request was only done for some exception I can't remember actually. IMO the custom fields values should be fetched from the item which get saved and not from the post request. That looks for me wrong.

I haven't changed that specific line, only the comment tied to it.
May it be that the custom fields data has been sanitised and removed prior to that step and thus we need to fetch it again? I haven't looked closer at it when I did this PR.

In most cases, the fields values are in $item->params which are validated by the form. The code is on line 68.

I had a look now, the line is needed.
The thing is that $item is a JTable object and thus only contains the data that is going to be stored into the respective database table. There is neither a params or com_fields property at this point. So we need to fetch the raw form data and take the custom fields out of it.

For contacts the $item->params property exists which holds the custom fields values. But you are right for articles not. What is missing here is the form validation when take it directly from the input.

True that. I'd say it's out of scope of this PR as this behavior exists with or without this PR.

ggppdk · 2016-12-28T12:03:31Z

This PR is needed
Besides the obvious reason to avoid conflicts, there are more benefits

since anyway they need to be seperated from params array, why add them there in the first place,

thus avoid the extra step to find the custom fields inside the 'params' and then clean-up the 'params' array before DB save

it makes easier to reference / handle them inside form via JS
easier to apply "different" validation of them ... in future if needed
also logically it makes sense since the 'params' are mostly non-content, and 'com_fields' are mostly 'content' ...

laoneo · 2017-01-26T07:48:23Z

We have conflicts here.

Bakual · 2017-01-26T08:29:56Z

Rebased and conflict solved.

Bakual · 2017-02-02T20:35:37Z

Rebased and fixed conflicts (again)

Show custom fields in frontend article edit using the joomla.edit.params JLayout. Adjusted the JLayout to take a custom tab name.

Bakual · 2017-02-26T21:09:22Z

Rebased and fixed conflicts once more

laoneo · 2017-02-27T18:06:44Z

-	 * @return  boolean
-	 *
-	 * @since   3.7.0
-	 */


Please don't delete that code. It cleans up the items which do have a params attribute. Otherwise the value of the fields are saved in the params array of the item as well.

The whole point of this PR is to not store the field data in the params property. So no, they aren't saved there since we don't populate it into params anymore. And thus that code isn't needed anymore. 😄

yvesh · 2017-03-03T19:11:48Z

 JHtml::_('formbehavior.chosen', '#jform_catid', null, array('disable_search_threshold' => 0));
 JHtml::_('formbehavior.chosen', 'select');
+$this->tab_name = 'com-content-form';
+$this->ignore_fieldsets = array('image-intro', 'image-full', 'jmetadata', 'item_associations');


Maybe a stupid question but why did you add this line? :-)

Because the article form currently only renders a fixed set of fieldsets. But we need it to render all available fieldsets.
So I have changed it to it renders all except those which are already hardcoded rendered.

coolcat-creations · 2017-03-08T13:44:14Z

Please tag me if ready to test :)

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

Bakual · 2017-03-08T15:08:23Z

@coolcat-creations It is ready for quite some time already 😄

ggppdk · 2017-03-08T15:19:10Z

I will test too during weekend

coolcat-creations · 2017-03-08T16:31:50Z

I have tested this item ✅ successfully on 2b5784c

@Bakual sorry i should have read my errormessage :-D Test successful!

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

laoneo · 2017-03-09T06:14:20Z

I will also have a look over the weekend, somehow I have the feeling something is missing.

laoneo · 2017-03-10T09:57:12Z

I have tested this item ✅ successfully on 2b5784c

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

ghost · 2017-03-10T10:25:26Z

RTC as there are 2 successfully Tests?

jeckodevelopment · 2017-03-10T10:53:37Z

RTC

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

joomla-cms-bot added the PR-staging label Dec 23, 2016

Bakual mentioned this pull request Dec 23, 2016

[com_fields] Remove alias from fields #13182

Closed

laoneo reviewed Dec 24, 2016

View reviewed changes

Bakual force-pushed the ChangeParamsToComfields branch from c8ff6d1 to 77c0799 Compare December 28, 2016 12:06

This was referenced Jan 17, 2017

[com_fields] Use timezone and format of the calendar field #13606

Closed

[com_fields] Remove unneeded fields from com_fields #13621

Merged

astridx mentioned this pull request Jan 20, 2017

[com_fields] Rename alias to name and add a check if name is unique #13576

Closed

Bakual force-pushed the ChangeParamsToComfields branch from 77c0799 to 66873f8 Compare January 26, 2017 08:29

Bakual force-pushed the ChangeParamsToComfields branch from 66873f8 to 9ef4ae5 Compare February 2, 2017 20:33

Bakual mentioned this pull request Feb 7, 2017

Fix showing user fields in a contact #13956

Merged

This was referenced Feb 15, 2017

[fields] Fix deprecated JUser::getParameters() #14082

Closed

[com_fields] Default params are being override #14146

Closed

Store fields in 'com_fields' property instead of reusing 'params'

75444ec

Show custom fields in frontend article edit using the joomla.edit.params JLayout. Adjusted the JLayout to take a custom tab name.

Bakual force-pushed the ChangeParamsToComfields branch from 9ef4ae5 to 75444ec Compare February 26, 2017 21:06

Removing unneeded code from onUserAfterSave method

2b5784c

laoneo reviewed Feb 27, 2017

View reviewed changes

yvesh reviewed Mar 3, 2017

View reviewed changes

Bakual mentioned this pull request Mar 10, 2017

System - Fields plugin: $fields property in onContentPrepare method #14437

Closed

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 10, 2017

jeckodevelopment added this to the Joomla 3.7.0 milestone Mar 10, 2017

rdeutz merged commit e72cb97 into joomla:staging Mar 10, 2017

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

Bakual deleted the ChangeParamsToComfields branch March 10, 2017 22:25

Uh oh!

Conversation

Bakual commented Dec 23, 2016 • edited by jeckodevelopment Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Documentation Changes Required

Uh oh!

ralain commented Dec 23, 2016

Uh oh!

Bakual commented Dec 24, 2016

Uh oh!

laoneo commented Dec 24, 2016

Uh oh!

Bakual commented Dec 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laoneo Dec 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ggppdk commented Dec 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laoneo commented Jan 26, 2017

Uh oh!

Bakual commented Jan 26, 2017

Uh oh!

Bakual commented Feb 2, 2017

Uh oh!

Bakual commented Feb 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bakual Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

coolcat-creations commented Mar 8, 2017

Uh oh!

Bakual commented Mar 8, 2017

Uh oh!

ggppdk commented Mar 8, 2017

Uh oh!

coolcat-creations commented Mar 8, 2017

Uh oh!

laoneo commented Mar 9, 2017

Uh oh!

laoneo commented Mar 10, 2017

Uh oh!

ghost commented Mar 10, 2017

Uh oh!

jeckodevelopment commented Mar 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Bakual commented Dec 23, 2016 •

edited by jeckodevelopment

Loading

laoneo Dec 28, 2016 •

edited

Loading

ggppdk commented Dec 28, 2016 •

edited

Loading

Bakual Feb 27, 2017 •

edited

Loading