Skip to content

Conversation

@joomdonation
Copy link
Contributor

Pull Request for Issue # .

Summary of Changes

(ChatGPT helped improving my original PR description to make it more clear)

This PR refactors the MVC FormController class to reduce duplicated logic and improve extensibility.
The current save() method performs many different tasks in a single large block, making it difficult for child controllers to override small parts of the workflow without re-implementing the entire method. This PR introduces several internal helper methods and reorganizes existing logic to make the controller easier to maintain and extend.

1. Extracted check-in / check-out logic

Repeated check-in and check-out code in the edit, cancel, and save tasks has been moved into two reusable methods:

  • attemptCheckin()
  • attemptCheckout()

This isolates record-locking behavior and allows child controllers to override it without touching unrelated logic.

2. Extracted calendar-field request filtering

The repeated logic for normalizing calendar form fields (applying SERVER_UTC or USER_UTC filters and writing the normalized values back to the request data) has been extracted into:

  • applyFilterForCalendarFieldsToRequestData()

This logic is used both when validation fails during save() and in the reload() task.

3. Split the large save() method into smaller steps

The original save() method contained many responsibilities (data preprocessing, plugin manipulation, permission checking, validation, saving, messages, redirection, check-in).
The method has now been broken into clearly named helper methods:

  • preprocessSaveData()
    Allows child controllers to adjust raw request data before any validation or permission checks happen.

  • normalizeRequestData()
    Extracted from existing logic. Allows plugins to modify request data before validation.

  • preSaveHook()
    Allows child controllers to modify the already-validated data before it is passed to the model for saving.

  • handleSaveDataValidationErrorMessages()
    Extracted from existing code to output validation errors.

  • setSaveSuccessMessage()
    Extracted from existing code to construct and set success messages after a successful save.

By splitting these steps, child controllers can now override only the parts they need — without copying the entire save() method.


Things to Consider / Open Questions

1. Correct type-hint for the model

FormController currently type-hints $model as BaseDatabaseModel.
However, some of the logic used by FormController (including check-in/check-out) is available only in:

  • FormModel, or
  • a model with a Table supporting check-in/check-out.

While refactoring, I kept BaseDatabaseModel for consistency with the existing code.
However, the more accurate type hint would be FormModel.

Question:
Should the new helper methods use FormModel as the parameter type?
If desired, I can adjust the PR accordingly.


2. FormController::save() assumes a database table always exists

The current implementation assumes that every item being edited has an associated Table object.
This is not always true — for example, the OverrideController in com_languages:

https://github.com/joomla/joomla-cms/blob/6.1-dev/administrator/components/com_languages/src/Controller/OverrideController.php#L72

OverrideController does not use a database table, so it cannot reuse FormController::save() at all.
Controllers without tables are forced to reimplement the entire save process.

Possible improvement (for a future PR):

  • Make the Table object optional inside FormController::save().
  • If no Table is available:
    • skip check-in/check-out,
    • skip table-specific operations,
    • still run validation, model save, messages, redirection.

This would allow table-less controllers (e.g., for configuration or overrides) to reuse the base controller logic.

Testing Instructions

  • Use Joomla 6.1, apply patch, try to add/edit, save as copy articles and make sure it still working as expected. But do not test this for now, I want to see if maintainers want to accept this change first.

Actual result BEFORE applying this Pull Request

  • Works. But the code is not good: repeating, not easy for child controllers to override to extend the logic

Expected result AFTER applying this Pull Request

  • Works, reduce repeating code, easier for child controller to override part of save method to add it own logic.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants