Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a getSaveData function in form controller for data src override #15331

Closed
wants to merge 5 commits into from

Conversation

julienV
Copy link
Contributor

@julienV julienV commented Apr 16, 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

@ghost ghost added the J3 Issue label Apr 5, 2019
@ghost ghost removed the J3 Issue label Apr 19, 2019
@SharkyKZ
Copy link
Contributor

SharkyKZ commented Apr 1, 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.

1 similar comment
@jwaisner
Copy link
Member

jwaisner commented Apr 7, 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.

@jwaisner
Copy link
Member

jwaisner commented Apr 7, 2020

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 7, 2020
@HLeithner
Copy link
Member

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.

@julienV
Copy link
Contributor Author

julienV commented Apr 9, 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.

@HLeithner
Copy link
Member

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.

@richard67
Copy link
Member

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

@julienV
Copy link
Contributor Author

julienV commented May 17, 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...

@HLeithner
Copy link
Member

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.

@HLeithner HLeithner closed this May 22, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 22, 2020
@julienV
Copy link
Contributor Author

julienV commented May 22, 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.

@SharkyKZ
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants