Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

Denitz
Copy link
Contributor

@Denitz Denitz commented Jun 6, 2022

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 Denitz changed the title fix JTable::store(true) to update NULLs fails if assets are tracked Jun 6, 2022
@richard67
Copy link
Member

richard67 commented Jun 11, 2022

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
Copy link
Member

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

@Denitz
Copy link
Contributor Author

Denitz commented Jun 14, 2022

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

@chmst
Copy link
Contributor

chmst commented Jun 15, 2022

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.

@HLeithner HLeithner changed the base branch from 4.1-dev to 4.2-dev June 27, 2022 13:04
@HLeithner
Copy link
Member

This pull requests has automatically rebased to 4.2-dev.

@joomla-bot
Copy link
Contributor

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

@HLeithner HLeithner removed the psr12 label Oct 23, 2022
@Hackwar Hackwar added the Small A PR which only has a small change label Feb 26, 2023
@Denitz
Copy link
Contributor Author

Denitz commented Mar 24, 2023

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

@Denitz
Copy link
Contributor Author

Denitz commented Mar 24, 2023

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.

@richard67 richard67 changed the base branch from 4.2-dev to 4.3-dev March 24, 2023 21:22
@HLeithner
Copy link
Member

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

@Denitz Denitz changed the title JTable::store(true) to update NULLs fails if assets are tracked [4.4] JTable::store(true) to update NULLs fails if assets are tracked Oct 3, 2023
@Hackwar Hackwar added the PBF Pizza, Bugs and Fun label Feb 21, 2024
@devcodemonkey
Copy link

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.

@TLWebdesign
Copy link

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
Copy link

I have tested this item 🔴 unsuccessfully on 54ac9e4

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.

@richard67
Copy link
Member

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

@TLWebdesign
Copy link

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

@richard67
Copy link
Member

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.

@Hackwar
Copy link
Member

Hackwar commented Feb 26, 2024

I have tested this item ✅ successfully on 54ac9e4

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 Hackwar added PR-4.3-dev and removed PBF Pizza, Bugs and Fun bug Small A PR which only has a small change PR-4.4-dev labels Feb 26, 2024
@Hackwar
Copy link
Member

Hackwar commented Feb 26, 2024

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 26, 2024
@richard67 richard67 added PBF Pizza, Bugs and Fun bug Small A PR which only has a small change PR-4.4-dev and removed PR-4.3-dev labels Feb 26, 2024
@MacJoom MacJoom self-assigned this Feb 26, 2024
@MacJoom MacJoom merged commit 3277eb8 into joomla:4.4-dev Feb 26, 2024
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 26, 2024
@MacJoom MacJoom added this to the Joomla! 4.4.4 milestone Feb 26, 2024
@MacJoom
Copy link
Contributor

MacJoom commented Feb 26, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Maintainers Checked Used if the PR is conceptional useful PBF Pizza, Bugs and Fun PR-4.4-dev Small A PR which only has a small change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet