joomla-cms
joomla-cms copied to clipboard
[4.4] JTable::store(true) to update NULLs fails if assets are tracked
Pull Request for Issue #30995.
Summary of Changes
Store assets without NULLs.
Testing Instructions
Create custom table class with assets (property $asset_id is required) and try to execute parent::store(true).
Or just edit libraries/src/Table/Content.php, replace at the bottom return parent::store($updateNulls); with return parent::store(true); to emulate the real case, next try to save an article.
Actual result BEFORE applying this Pull Request
See error.
Expected result AFTER applying this Pull Request
Save without errors.
Documentation Changes Required
No.
By review I can confirm that this PR here fixes it in the right way like we already have it at other places where we store assets:
- https://github.com/joomla/joomla-cms/blob/4.1-dev/libraries/src/Installer/Adapter/ComponentAdapter.php#L424
- https://github.com/joomla/joomla-cms/blob/4.1-dev/administrator/components/com_admin/script.php#L7909
- https://github.com/joomla/joomla-cms/blob/4.1-dev/administrator/components/com_config/src/Model/ApplicationModel.php#L437
- https://github.com/joomla/joomla-cms/blob/4.1-dev/administrator/components/com_config/src/Model/ApplicationModel.php#L1148
- https://github.com/joomla/joomla-cms/blob/4.1-dev/administrator/components/com_config/src/Model/ComponentModel.php#L207
Hmm, but when doing a real test as suggested to emulate the real case, I do not get any error without this PR applied.
@Denitz As I wrote in my previous comments, I think by review that your PR is right. But I can't really reproduce the issue on a current 4.1-dev. I do not get any kind of error without the PR applied and doing the suggested edit of the "libraries/src/Table/Content.php" file. Could you check and report back how you did get an error and which kind of error?
Sorry, can't reproduce as well, too much time has passed :(
Can replicate following this instruction, but get another message.
Or just edit libraries/src/Table/Content.php, replace at the bottom return parent::store($updateNulls); with return parent::store(true); to emulate the real case, next try to save an article.
BUT .. I could replicate it only once and never again.
This pull requests has automatically rebased to 4.2-dev.
This pull requests has been automatically converted to the PSR-12 coding standard.
@richard67 Please rebase to 4.3-dev, I don't want to loose your approval after my rebase.
Anybody, please delete this PR if I won't remind about it in 5 years, otherwise it means that I've gone to better place.
This pull request has been automatically rebased to 4.4-dev.
I have tested this item :white_check_mark: successfully on 54ac9e42a08336b9985ce4cd624bcd3fcd06be19
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37993.
I have tested this item ✅ successfully on 54ac9e4
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37993.
What did you do to succesfully get the error message? I am not getting any errors before applying the patch.
I have tested this item :red_circle: unsuccessfully on 54ac9e42a08336b9985ce4cd624bcd3fcd06be19
Could not replicate the error on saving before applying the patch
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37993.
@TLWebdesign The testing instructions say that it needs to implement a custom table class to reproduce the issue. That means some programming or some 3rd party extension which does that. Have you done that? If no, then change your test result to "not tested". Thanks in advance.
@richard67
it also said you could do this approach:
Or just edit libraries/src/Table/Content.php, replace at the bottom return parent::store($updateNulls); with return parent::store(true); to emulate the real case, next try to save an article.
Thats what i tried.
it also said you could do this approach:
Or just edit libraries/src/Table/Content.php, replace at the bottom return parent::store($updateNulls); with return parent::store(true); to emulate the real case, next try to save an article.
Thats what i tried.
I see. Was reading not far enough.
I have tested this item :white_check_mark: successfully on 54ac9e42a08336b9985ce4cd624bcd3fcd06be19
I did a codereview and indeed it is false to hand over the $updateNulls here to the asset table object. So this can be merged by codereview.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37993.
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37993.
Thank you!