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.

Sign up for GitHub

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

Jump to bottom

BUGFIX: Assign original asset collections to variant #3562

Merged
merged 6 commits into from Aug 1, 2023
Merged

BUGFIX: Assign original asset collections to variant #3562

merged 6 commits into from Aug 1, 2023

Conversation

kdambekalns
Copy link
Member

Fixes #3560

@kdambekalns kdambekalns self-assigned this Jan 11, 2022
@bwaidelich
Copy link
Member

@kdambekalns is this still marked as draft because you are working on a test case? *g

@kdambekalns
Copy link
Member Author

@kdambekalns is this still marked as draft because you are working on a test case? *g

No, because… see linked issue. ;)

@bwaidelich
Copy link
Member

Ah right.. good question..

@kdambekalns
Copy link
Member Author

Conclusion to above mentioned question: makes sense to keep the collections in sync…

@ru3fu5z
Copy link
Contributor

ru3fu5z commented Feb 3, 2023

Will this be fixed soon? #3560 (comment)

@kdambekalns kdambekalns marked this pull request as ready for review April 29, 2023 09:11
@kdambekalns kdambekalns changed the base branch from 5.3 to 7.3 April 29, 2023 09:36
fcool
fcool approved these changes Apr 29, 2023
View reviewed changes
Copy link
Contributor

@fcool fcool left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks definetly ripe enough for merging

mhsdesign
mhsdesign previously requested changes Jun 23, 2023
View reviewed changes
Copy link
Member

@mhsdesign mhsdesign left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style ci stuff imo doesnt belong into this specific bugfix branch (and might cause trouble, i just have an ungood feeling ^^). I would like 4559130 to be reverted.

@kdambekalns can you give me a 👍 if you agree? then id go ahead and do so.

@mhsdesign mhsdesign disabled auto-merge June 23, 2023 18:27
@kdambekalns
Copy link
Member Author

The style ci stuff imo doesnt belong into this specific bugfix branch (and might cause trouble, i just have an ungood feeling ^^)

  • Might belong elsewhere, but if we always ignore those, they will never be fixed.
  • What kind of trouble do you expect?

@kdambekalns can you give me a 👍 if you agree? then id go ahead and do so.

I disagree, that's a "pure whitespace" change, I see no need to revert that.

@mhsdesign
Copy link
Member

What kind of trouble do you expect?

The style ci stuff was done some time ago Apr 29, and might be outdated - also if every bugfix branch does it its just odd ^^.

I also thought our style ci was wrong configured as it only complains now (the code wasnt touched in years)

@kdambekalns
Copy link
Member Author

also if every bugfix branch does it its just odd ^^.

No. You open a PR, the checks fail, you fix them. Nothing odd about that, IMHO.

I also thought our style ci was wrong configured as it only complains now (the code wasnt touched in years)

That is a "known issue" of some sort. But either way, the complaints are valid.

@mhsdesign mhsdesign mentioned this pull request Jun 27, 2023
Closed
@mhsdesign mhsdesign force-pushed the bugfix/3560-assign-originalasset-assetcollections-to-imagevariant branch from 4559130 to b068c8e Compare June 27, 2023 08:38
@mhsdesign mhsdesign dismissed their stale review June 27, 2023 08:41

Okay the style ci changes from april 29 were outdated, but i fixed the new ones on 7.3 with #4370 and cleaned up the commit from your pr which i disliked ^^

@kdambekalns
Copy link
Member Author

Can't say I am a fan of force-pushing over other people's work without previous agreement. Especially when it's because something is disliked. 🙁

Thanks for housekeeping, still.

@crydotsnake crydotsnake removed their request for review June 28, 2023 09:32
@mhsdesign
Copy link
Member

Sorry not especially nice of me

@ru3fu5z
Copy link
Contributor

ru3fu5z commented Jul 24, 2023 •
edited

Any news on this PR? @kdambekalns @mhsdesign

@kdambekalns
Copy link
Member Author

Waiting for reviews… sadly.

kitsunet
kitsunet approved these changes Jul 24, 2023
View reviewed changes
Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess works for me.

@kdambekalns kdambekalns merged commit 5fdf5f3 into neos:7.3 Aug 1, 2023
8 checks passed
@dlubitz dlubitz mentioned this pull request Nov 23, 2023
Merged
@lorenzulrich lorenzulrich mentioned this pull request Dec 28, 2023
Closed
Sebobo added a commit to Flowpack/media-ui that referenced this pull request Dec 28, 2023
@Sebobo
BUGFIX: Don’t include asset variants in counts
a150e0f 
This problem appeared due to variants getting the same collection applied as their original asset in neos/neos-development-collection#3562

Fixes: #224
@Sebobo Sebobo mentioned this pull request Dec 28, 2023
Merged
Sebobo added a commit to Flowpack/media-ui that referenced this pull request Jan 8, 2024
@Sebobo
BUGFIX: Don’t include asset variants in counts
1221c4a 
This problem appeared due to variants getting the same collection applied as their original asset in neos/neos-development-collection#3562

Fixes: #224
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fcool fcool fcool approved these changes

@kitsunet kitsunet kitsunet approved these changes

@robertlemke robertlemke Awaiting requested review from robertlemke

@Sebobo Sebobo Awaiting requested review from Sebobo

@daniellienert daniellienert Awaiting requested review from daniellienert

@markusguenther markusguenther Awaiting requested review from markusguenther

@bwaidelich bwaidelich Awaiting requested review from bwaidelich

@mficzel mficzel Awaiting requested review from mficzel

@mhsdesign mhsdesign Awaiting requested review from mhsdesign

@ahaeslich ahaeslich Awaiting requested review from ahaeslich

Assignees

@kdambekalns kdambekalns

Labels
7.3 Bug P: Media T: PHP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@kdambekalns @bwaidelich @ru3fu5z @mhsdesign @fcool @kitsunet