Pending

User tests: Successful: 0 Unsuccessful: 0

avatar felixkat
felixkat
7 Feb 2025

This change would run the "Trigger the before save event" before "checking the data". This allows 3rd parties to sanitize their data before Joomla performs the checks before storing.

Pull Request for Issue # . 44795

Summary of Changes

Currently various models are:-

  1. Binding the data,
  2. Preparing for saving
  3. Checking the data
  4. Triggering onBeforeSave
  5. Storing.

This change swaps around 3 & 4 so 3rd parties can sanitize their data before Joomla performs the check.

Testing Instructions

Actual result BEFORE applying this Pull Request

Error messages such as "The content exceeds the allowed limits." without the ability to save.

Expected result AFTER applying this Pull Request

Saves are successful.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

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

  • Pull Request link for manual.joomla.org:

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

avatar felixkat felixkat - open - 7 Feb 2025
avatar felixkat felixkat - change - 7 Feb 2025
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 7 Feb 2025
Category Administration com_categories com_languages com_menus com_modules com_tags com_templates com_users Libraries
avatar felixkat felixkat - change - 7 Feb 2025
Title
https://github.com/joomla/joomla-cms/issues/44795
[5.2] Change the order of the 'safety check' and 'onExtensionBeforeSave'?
avatar felixkat felixkat - edited - 7 Feb 2025
avatar felixkat
felixkat - comment - 7 Feb 2025

@SharkyKZ Can you explain the reason for the thumbs down to get a understanding why it may not be a viable change?

avatar joomdonation
joomdonation - comment - 8 Feb 2025

Your propose change is backward incomptable and could cause issues with third party plugins :

  • With the current process, plugins which handle event_before_save event receive $table object with it's data already processed by $table->check() method
  • With your propose change, plugins which handle event_before_save event receive $table object with it's data haven't processed by by $table->check() method yet

I'm afraid of this change won't be accepted and you will need to find a different way to solve the problem described in your issue.

avatar felixkat
felixkat - comment - 8 Feb 2025

With your propose change, plugins which handle event_before_save event receive $table object with it's data haven't processed by by $table->check() method yet

@joomdonation Well that's kind of the point of the change. It doesn't seem to make sense to check the table and then be able to manipulate the data, via event_before_save, before saving it. At that point you are not saving the checked data.

Surely Joomla's "check" should be the final one before storing?

avatar joomdonation
joomdonation - comment - 8 Feb 2025

It doesn't seem to make sense to check the table and then be able to manipulate the data, via event_before_save, before saving it. At that point you are not saving the checked data

I see event_before_save allows doing final validation/ final things before the item is stored into database, not for manipulating data. Data sanitize/validation happens earlier during process, and we have several event triggers for that already:

avatar felixkat
felixkat - comment - 8 Feb 2025
  • With your propose change, plugins which handle event_before_save event receive $table object with it's data haven't processed by by $table->check() method yet

Data sanitize/validation happens earlier during process, and we have several event triggers for that already:

In our scenario, with a module, the params data exceeds the 65535 limit which fails the table->check(). The data is valid but we store it as a json file, this process happens during event_before_save in addition to clearing the database params value. The table->check() check prevents us from getting to the point of doing that.

The current event order could potentially allow a 3rd party to incorrectly assign data after the safety check leading to truncated or corrupt data, which to me isn't logical.

I am trying to see it from all angles but perhaps I'm misinterpreting the above quoted comments as conflicting.

Add a Comment

Login with GitHub to post a comment