Skip to content

IBX-10221: Fixed 500 error when removing Site skeletons by improving location root checks #1625

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
Merged
Parent: 4.6
merged 7 commits into from
Jul 15, 2025
Merged

IBX-10221: Fixed 500 error when removing Site skeletons by improving location root checks #1625

merged 7 commits into from
Jul 15, 2025

Conversation

@tbialcz
Copy link
Contributor

@tbialcz tbialcz commented Jul 8, 2025 •
edited
Loading

🎫 Issue IBX-10221

Description:

Fix 500 error when attempting to trash root-level or contextual root locations

  • Fixed a 500 error that occurred when attempting to send a Site skeleton to trash, caused by incorrect handling of contextual root locations.
  • Introduced the IsInContextualTreeRootIds specification, which checks whether a location belongs to the configured contextual root locations via the
  • The "Send to Trash" menu item is now hidden when the selected location is either the root or a contextual root location.

For QA:

Documentation:

@tbialcz tbialcz requested a review from a team July 8, 2025 09:50
@ezrobot ezrobot requested review from Steveb-p, ViniTou, adamwojs, alongosz, barw4, ciastektk, konradoboza, mikadamczyk and wiewiurdp and removed request for a team July 8, 2025 09:50
mikadamczyk
mikadamczyk reviewed Jul 8, 2025
View reviewed changes
@tbialcz tbialcz requested a review from mikadamczyk July 8, 2025 14:20
konradoboza
konradoboza requested changes Jul 9, 2025
View reviewed changes
@konradoboza
Copy link
Contributor

@tbialcz could you please add ibexa/phpstan first to check if accessing config resolver parameters in constructor is caught? This is pretty simple, please look at https://github.com/ibexa/storefront/pull/180/files.

@tbialcz
Copy link
Contributor Author

tbialcz commented Jul 9, 2025

@tbialcz could you please add ibexa/phpstan first to check if accessing config resolver parameters in constructor is caught? This is pretty simple, please look at https://github.com/ibexa/storefront/pull/180/files.

Confirmed it works:
Line src/lib/Specification/Location/IsContentStructureRoot.php

20 Referring to ConfigResolver parameters in constructor is not allowed due to potential scope change.

@tbialcz tbialcz requested a review from konradoboza July 9, 2025 06:33
konradoboza
konradoboza approved these changes Jul 9, 2025
View reviewed changes
Copy link
Contributor

@konradoboza konradoboza left a comment

Choose a reason for hiding this comment

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

Thanks @tbialcz, looks better now!

konradoboza
konradoboza reviewed Jul 9, 2025
View reviewed changes
@katarzynazawada katarzynazawada mentioned this pull request Jul 9, 2025
Closed
katarzynazawada
katarzynazawada requested changes Jul 9, 2025
View reviewed changes
Copy link
Contributor

@katarzynazawada katarzynazawada left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Site skeletons folder looks fine - cannot remove it via UI. But with this change, the possibility of deletion for non-root items has disappeared - the Send to trash button is no longer available in the context menu. Move button is disabled.
Regression fails: https://github.com/ibexa/commerce/actions/runs/16166287562/job/45628876549?pr=1387#step:18:187
Screenshot 2025-07-09 at 12 15 25

@tbialcz
Copy link
Contributor Author

tbialcz commented Jul 11, 2025

Site skeletons folder looks fine - cannot remove it via UI. But with this change, the possibility of deletion for non-root items has disappeared - the Send to trash button is no longer available in the context menu. Move button is disabled. Regression fails: https://github.com/ibexa/commerce/actions/runs/16166287562/job/45628876549?pr=1387#step:18:187 Screenshot 2025-07-09 at 12 15 25

Fixed

tbialcz added 7 commits July 14, 2025 10:42
@tbialcz @katarzynazawada
IBX-10221: Fixed 500 error when removing Site skeletons by improving …
81deb01 
…location root checks
@tbialcz @katarzynazawada
IBX-10221: Improved location checks by introducing IsContentStructure…
8f3d5d6 
…Root specification
@tbialcz @katarzynazawada
IBX-10221: Refactored IsContentStructureRoot specification and added …
87dce1c 
…ibexa/phpstan dependency
@tbialcz @katarzynazawada
IBX-10221: Replaced direct depth property access with getDepth() and …
c83ea8c 
…added unit tests for IsContentStructureRoot specification
@tbialcz @katarzynazawada
IBX-10221: Simplified Location mock creation in tests and added unit …
48e0c2b 
…tests for IsRoot specification
@tbialcz @katarzynazawada
IBX-10221: Marked test classes as final and replaced assert calls wit…
635e40a 
…h static assertions
@tbialcz @katarzynazawada
IBX-10221: Replaced IsContentStructureRoot with IsInContextualTreeRoo…
7be4bbb 
…tIds specification and added corresponding tests
@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@adamwojs adamwojs merged commit a80c7d8 into 4.6 Jul 15, 2025
28 checks passed
@adamwojs adamwojs deleted the ibx_10221 branch July 15, 2025 12:16
@adamwojs
Copy link
Member

@tbialcz Could you please merge up changes ?

@tbialcz
Copy link
Contributor Author

tbialcz commented Jul 15, 2025

@tbialcz Could you please merge up changes ?

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikadamczyk mikadamczyk mikadamczyk approved these changes

@wiewiurdp wiewiurdp wiewiurdp approved these changes

@konradoboza konradoboza konradoboza approved these changes

@katarzynazawada katarzynazawada katarzynazawada approved these changes

@adamwojs adamwojs Awaiting requested review from adamwojs

@Steveb-p Steveb-p Awaiting requested review from Steveb-p

@alongosz alongosz Awaiting requested review from alongosz

@ciastektk ciastektk Awaiting requested review from ciastektk

@ViniTou ViniTou Awaiting requested review from ViniTou

@barw4 barw4 Awaiting requested review from barw4

Assignees

@mikadamczyk mikadamczyk

@ciastektk ciastektk

@ViniTou ViniTou

Labels

QA approved Ready for review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@tbialcz @konradoboza @adamwojs @mikadamczyk @wiewiurdp @katarzynazawada @ciastektk @ViniTou