joomla-cms icon indicating copy to clipboard operation
joomla-cms copied to clipboard

[6.1] Add redirect URLs utility methods for FormController

Open joomdonation opened this issue 4 weeks ago • 12 comments

Pull Request for Issue # .

Summary of Changes

This PR adds two new methods, getRedirectUrlToItem() and getRedirectUrlToList(), to the FormController class. These methods centralize the generation of redirection URLs and replace repeated code blocks inside the class.

If accepted, this approach can be extended to child classes of FormController, reducing code duplication and improving maintainability.

Testing Instructions

  • Use Joomla 6.1
  • Apply patch
  • Try to add/edit article and make sure everything is still working as expected
  • On Articles Management screen, try to publish/unpublish/delete articles and make sure it still works like before

Actual result BEFORE applying this Pull Request

Works, but there is repeating code

Expected result AFTER applying this Pull Request

Cleaner, shorter code and can be used by child classes

Link to documentations

Please select:

  • [ ] Documentation link for docs.joomla.org:

  • [ ] No documentation changes for docs.joomla.org needed

  • [ ] Pull Request link for manual.joomla.org:

  • [ ] No documentation changes for manual.joomla.org needed

joomdonation avatar Dec 03 '25 08:12 joomdonation

We have the same "issue" in the AdminController, not sure if FromController is the right place to implement this.

HLeithner avatar Dec 03 '25 10:12 HLeithner

Yes, I’m aware of that. However, since AdminController does not extend FormController, there isn’t a straightforward way to share these methods between them. One alternative would be to introduce a trait for these reusable methods, but adding a trait for just one or two methods might not be justified at this stage.

Please note that AdminController already contains its own implementation of a similar method (https://github.com/joomla/joomla-cms/blob/6.1-dev/libraries/src/MVC/Controller/AdminController.php#L479), which mirrors the one in FormController (https://github.com/joomla/joomla-cms/blob/6.1-dev/libraries/src/MVC/Controller/FormController.php#L506). That means that the two controller classes were designed to be independent. So maybe the two methods can stay here in FormController, then we can implement getRedirectToListUrl method to AdminController, too.

joomdonation avatar Dec 03 '25 11:12 joomdonation

if it should not be in the base class then a trait is perfectly fine for this.

HLeithner avatar Dec 03 '25 11:12 HLeithner

The trait would then only contains getRedirectToListUrl because AdminController does not need getRedirectToItemUrl. I'm unsure if that makes much sense.

joomdonation avatar Dec 03 '25 11:12 joomdonation

you might be right, then please add the getRedirectToListUrl to the also to the admincontroller

HLeithner avatar Dec 03 '25 11:12 HLeithner

you might be right, then please add the getRedirectToListUrl to the also to the admincontroller

Done, thanks ! FYI, I see many possible improvements to FormController code (especially for save method) to make easier to override when it is needed. As of right now, many child controllers in have to copy/paste the whole code of the method (for example https://github.com/joomla/joomla-cms/blob/6.1-dev/administrator/components/com_finder/src/Controller/FilterController.php#L40) to add it own logic which is not nice. We should improve it.

joomdonation avatar Dec 03 '25 12:12 joomdonation

that's true, if you find a nice way to split the save function into multiple so easier overriding is possible please go for it.

HLeithner avatar Dec 03 '25 16:12 HLeithner

I cannot speak about whether it is needed. But I would suggest to use different naming, like getRedirectUrl...(), getRedirectUrlFoo(), getRedirectUrlBar(). It should be more easy to search and read in my opinion.

Fedik avatar Dec 04 '25 17:12 Fedik

I cannot speak about whether it is needed.

The two methods provide quicker way to get the URLs instead of having to repeat writing longer code to build these URLs everywhere (the two classes modified in this PR are just a start, there are many code like that in child classes)

But I would suggest to use different naming, like getRedirectUrl...(), getRedirectUrlFoo(), getRedirectUrlBar()

I'm open for suggestion. In my extensions, I use getViewListUrl and getViewItemUrl. I think the suggested getRedirectUrl does not really describe what the methods do.

joomdonation avatar Dec 04 '25 17:12 joomdonation

I think the suggested getRedirectUrl does not really describe what the methods do.

I just mean "flip words" around getRedirectToItemUrl => getRedirectUrlToItem :wink:

Fedik avatar Dec 04 '25 19:12 Fedik

I just mean "flip words" around getRedirectToItemUrl => getRedirectUrlToItem

Thanks, it's indeed better, but having a little naming conflict with the existing method getRedirectToItemAppend. But I changed the name as suggested.

joomdonation avatar Dec 05 '25 03:12 joomdonation

that's true, if you find a nice way to split the save function into multiple so easier overriding is possible please go for it.

@HLeithner PR created https://github.com/joomla/joomla-cms/pull/46537. I also include changes from this PR in that PR to make the code shorter/more clear.

joomdonation avatar Dec 05 '25 08:12 joomdonation