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: Make getParent work in NodeData #4295

Merged
merged 1 commit into from Aug 17, 2023

Conversation

pKallert
Copy link
Contributor

@pKallert pKallert commented May 26, 2023

resolved: #4283

Review instructions
I am unsure why the dimensions were not passed to findOneByPath, but without it it does not really work.
Also, it is kind of strange that the dimensions in findOneByPath is optional but returns nothing if it is not set.
This fixes it for now.

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

@kitsunet
Copy link
Member

Thanks! I actually started writing an answer into the ticket earlier, I think it's fine to fix it here, the NodeData::getParent() is not used anywhere in typical Neos projects and I think this is a holdover from way early days. Technically given that NodeData has no concept of context it doesn't know much about dimensions, but I think it's fine to at least pass the ones it knows about here, as it should be fairly safe to get the parent based on that. Although we do make use of a quirk here in that dimensionValues is in all regular use cases a one to one relation despite the array (that is one dimension will always only have one value, you won't find a NodeData with language=>de,en for example). This quirk however holds only as long as no one uses the PHP API which indeed would allow this. My only fear is that if someone does this, it's not deterministic which parent they might get through this. But I would say the risk is minimal and we should rather add the fix...

@pKallert
Copy link
Contributor Author

@kitsunet I only used the NodeData->getParent function because the TransformationInterface uses the NodeData and not the NodeInterface.
Would it make sense to switch out the NodeData with NodeInterface in the TransformationInterface in the future?

@kitsunet
Copy link
Member

It is that way because we need to ensure that you touch every entity in the DB once and only once, you don't want to accidentally create variants in a migration or anything. So we do this on NodeData level as that is the entity actually seen in the DB. also allows easier iteration. If we could find a great concept for using Nodes directly that works for me, but given all that changes for Neos 9 anywasy, I am not sure it still makes sense to invest too much time into it.

@markusguenther markusguenther merged commit f7c72ea into neos:7.3 Aug 17, 2023
8 of 9 checks passed
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.

None yet

3 participants