? ? Success

User tests: Successful: Unsuccessful:

avatar julienV
julienV
16 Apr 2017

Summary of Changes

This is a very simple change to allow for form controller subclasses to easily override the source of data.
Currently, you have to rewrite the full save function if you want to add data other than jform post data.
For example for file uploads, a commonly found workaround, is to use the prepareTable function of the model, but i think it would be much cleaner to pull the data from controller, and not the model.

Testing Instructions

It shouldn't change anything as long as there is no override, so testing is just saving any form using the regular form controller (e.g: an article)

Expected result

works as usual

Actual result

works...

Documentation Changes Required

Changes nothing for end user

avatar julienV julienV - open - 16 Apr 2017
avatar julienV julienV - change - 16 Apr 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 16 Apr 2017
Category Libraries
avatar rdeutz rdeutz - change - 27 May 2017
Labels Added: ?
avatar SharkyKZ
SharkyKZ - comment - 1 Apr 2020

I have tested this item successfully on f16ab9d


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15331.

avatar SharkyKZ SharkyKZ - test_item - 1 Apr 2020 - Tested successfully
avatar jwaisner
jwaisner - comment - 7 Apr 2020

I have tested this item successfully on f16ab9d


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15331.

avatar jwaisner jwaisner - test_item - 7 Apr 2020 - Tested successfully
avatar jwaisner jwaisner - change - 7 Apr 2020
Status Pending Ready to Commit
avatar jwaisner
jwaisner - comment - 7 Apr 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15331.

avatar HLeithner
HLeithner - comment - 9 Apr 2020

3rd party developer using this function name already may have a b/c break, also reading the code correctly this could be done in the jmodel->validate function, actually I think it's the correct place.

avatar julienV
julienV - comment - 9 Apr 2020

I don't think so, it's the controller role to get the data from input, not the model's.
I know joomla models are already doing it from the populatestate function, doesn't make it right though.
see the original description above too.

avatar HLeithner
HLeithner - comment - 9 Apr 2020

There is another way to modify the data with a event, I'm not sure if we really need 3 ways to do the same thing.

avatar richard67
richard67 - comment - 17 May 2020

@HLeithner What shall we do with this PR? Remove RTC? Set any other label?

avatar julienV
julienV - comment - 17 May 2020

you have to be more explicit about what your solution is @HLeithner
again, for me the right place to get the data from input is the controller, not the model. You know, as in MVC architecture...

avatar HLeithner
HLeithner - comment - 22 May 2020

Beside the inconsistency in this PR ($recordId = $this->input->getInt($urlVar); is still loaded from JInput) I see not a good reason to add this to core, in J3 we could have a b/c break with 3rd party extensions (using the function name). For feature use we could have 100s of such functions and this would reduce performance without a good reason. If you like to modify the input then you can override the save function and change the input value or do any other of the options already said.

Which maybe makes for sense to split this 300 lines function into more logical parts to get better readability and reusability.

I'm closing this for now, thx for your engagement.

avatar HLeithner HLeithner - change - 22 May 2020
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2020-05-22 10:01:12
Closed_By HLeithner
Labels Added: ?
avatar HLeithner HLeithner - close - 22 May 2020
avatar julienV
julienV - comment - 22 May 2020

that was the point that it's a shame to have to override the whole controller save function for just the input... but ok, whatever.

avatar SharkyKZ
SharkyKZ - comment - 22 May 2020

@julienV You could try doing this against 4.0. More chances to get it in there...

Add a Comment

Login with GitHub to post a comment