@test edit a module from frontend still works with this PR

@zero-24 Done. Space removed.

Thanks so we need only one more tester/PLT to merge. Travis fails here unrelated to this PR. This PR is ok. Thanks!

Test successful.

Thanks for testing. As this bug can be seen easily by looking at the code and we now have two success tests, I hope PLT will accept and merge it.

RTC Thanks for your tests

Sorry to crash this party but the fix is fine but incomplete. When I save the module on frontend I indeed no longer get the fatal error. However, the error message is displayed as JERROR_SAVE_FAILED so we are missing the translation here. This string is available in the backend language file but not the frontend. This will need to be added as well.

Watch out, this is a string with a replacement tag %s. So the error message needs to be shown as well. This may require a bit more work. Please update the PR with the language string as well. Thank you.

You're right, didn't notice that :(

I think we can replace it with JLIB_APPLICATION_ERROR_SAVE_FAILED language item, seems it is the same with JERROR_SAVE_FAILED and it is loaded in the frontend as well

However, the difficulty here is that how we can get the detail error? The code in the frontend call a backend controller to save module and I don't know/don't see a way to access to the model to call getError method to get the error. Anyone knows a way to do that?

@brianteeman Hey Brian, I am pulling you in here because we have a language issue. As @joomdonation stated JLIB_APPLICATION_ERROR_SAVE_FAILED and JERROR_SAVE_FAILED are basically the same. How about we eliminate one of them?

@joomdonation I have looked into this on how to get the error message but there is no easy way of doing that because the save controller is in the backend and we are in the frontend. They are extending different baseclasses. The only idea I have is set the error also in the userState and check that perhaps. This is an untested idea ;)

We cant remove any string. It was previously decided that we do not remove
strings in case others are using them

On 5 May 2015 at 10:11, RolandD notifications@github.com wrote:

@brianteeman https://github.com/brianteeman Hey Brian, I am pulling you
in here because we have a language issue. As @joomdonation
https://github.com/joomdonation stated
JLIB_APPLICATION_ERROR_SAVE_FAILED and JERROR_SAVE_FAILED are basically the
same. How about we eliminate one of them?

@joomdonation https://github.com/joomdonation I have looked into this
on how to get the error message but there is no easy way of doing that
because the save controller is in the backend and we are in the frontend.
They are extending different baseclasses. The only idea I have is set the
error also in the userState and check that perhaps. This is an untested
idea ;)

—
Reply to this email directly or view it on GitHub
#6825 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Luckily, after reviewing the code again, I see that when an error happens, controller class also storing error message as well, so we just need to call $controllerClass->getError() to get the error message.

Could @roland-d and @OctavianC help testing it again?

After digging a little deeper, it seems that ModulesControllerModule doesn't return the save() result so the Fatal Error will never be encountered in production (it's always going to be null). Also, I'm not sure why ConfigControllerModulesSave is using the controller ModulesControllerModule instead of the model ModulesModelModule? I thought the whole logic of MVC was for the controller to call the model. The model also stores the error so we can use that too.

@OctavianC: I was thinking the same. And added return command in my previous commit.

For your second question, I guess the developer who wrote that code want to re-use the code and I don't like it, too. However, for now, I think we can leave it as how it is. If we want to re-factor it, I think we should wait until we have new MVC in Joomla core.

@joomdonation
So I have been trying to test this change but I can't get an error message to display. What I tried was emptying the value of a required field so the field validation would fail and tell me that I need to fill in the required field. What happens in reality is that the form comes back with no message at all because the $controllerClass has no errors.

The file I changed for testing is libraries/joomla/form/form.php at line 1957 just before that I put

$value = '';

Could you give step-by-step test instructions?

@brianteeman Thank you for your input.

@roland-d For some reasons, I could not get the error messages to display, too. When the form validation is false, all the validation error messages will be queued https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/controller/form.php#L708 and should be displayed

Not sure why it is not displayed with frontend module editing :(.

@joomdonation This is very intriguing. When I have some time I will take a closer look at it.