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

[4.4] JTable::store(true) to update NULLs fails if assets are tracked

Open Denitz opened this issue 3 years ago • 16 comments

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.

Denitz avatar Jun 06 '22 06:06 Denitz

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

richard67 avatar Jun 11 '22 09:06 richard67

Hmm, but when doing a real test as suggested to emulate the real case, I do not get any error without this PR applied.

richard67 avatar Jun 11 '22 10:06 richard67

@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?

richard67 avatar Jun 13 '22 13:06 richard67

Sorry, can't reproduce as well, too much time has passed :(

Denitz avatar Jun 14 '22 09:06 Denitz

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.

chmst avatar Jun 15 '22 06:06 chmst

This pull requests has automatically rebased to 4.2-dev.

HLeithner avatar Jun 27 '22 13:06 HLeithner

This pull requests has been automatically converted to the PSR-12 coding standard.

joomla-bot avatar Jun 27 '22 20:06 joomla-bot

@richard67 Please rebase to 4.3-dev, I don't want to loose your approval after my rebase.

Denitz avatar Mar 24 '23 21:03 Denitz

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.

Denitz avatar Mar 24 '23 21:03 Denitz

This pull request has been automatically rebased to 4.4-dev.

HLeithner avatar Sep 30 '23 22:09 HLeithner

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.

devcodemonkey avatar Feb 24 '24 13:02 devcodemonkey

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.

TLWebdesign avatar Feb 24 '24 13:02 TLWebdesign

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 avatar Feb 24 '24 13:02 TLWebdesign

@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 avatar Feb 24 '24 13:02 richard67

@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.

TLWebdesign avatar Feb 24 '24 13:02 TLWebdesign

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.

richard67 avatar Feb 24 '24 13:02 richard67

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.

Hackwar avatar Feb 26 '24 10:02 Hackwar

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37993.

Hackwar avatar Feb 26 '24 10:02 Hackwar

Thank you!

MacJoom avatar Feb 26 '24 15:02 MacJoom