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

BUGFIX: Build URI path based on parent from "target" dimensionSpacePoint #5091

Open
wants to merge 1 commit into
base: 9.0
Choose a base branch
from

Conversation

kdambekalns
Copy link
Member

When creating a node variant via switching of dimensions, the URI path projection did not take the URI (path) of the (new) parent into account. The URI was the "original" URI, regardless of the potentially differing URI path segments in the variants of the "new parents".

Now a node variant has a URL built from it's parent(s) in it's own dimensions.

Fixes #5090

Upgrade instructions

If affected by the bug, you can run

./flow cr:projectionreplay 'Neos\Neos\FrontendRouting\Projection\DocumentUriPathProjection'

to fix the projection.

Review instructions

Reproduction is described in the issue.

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

Very good catch and the fix makes sense to me.
I just added a comment re unified code style.

Apart from that we should cover this behavior with a corresponding test 😬

Comment on lines +455 to +458
$uriPathSegment = basename($sourceNode->getUriPath());
$uriPath = $parentNode->getUriPath() === ''
? $uriPathSegment
: $parentNode->getUriPath() . '/' . $uriPathSegment;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how we "calculate" the uri path in other places, but I don't remember having used basename() (not that I dislike it, but IMO it should be unified – potentially centralized to some DocumentNodeInfo::withUriPathSegment(string $newSegment)!?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, withUriPathSegment(…) does not help here, the basename(…) is used to get the segment I would need to pass here.

But I will get rid of basename and use code like in whenNodePropertiesWereSet(…) later in the class.

@kdambekalns
Copy link
Member Author

Yeah, I'll add a test. And I am confused, because I saw that work yesterday, and today it does no longer? 🤔

@mhsdesign
Copy link
Member

Hi what is the status here? ^^

@kdambekalns
Copy link
Member Author

Good question, lost track of this one… Will check!

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

Successfully merging this pull request may close these issues.

BUG: "Adopting" a node ignores URI of parent(s)
3 participants