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: Force direct access on setting node properties in node data similarize #5281

Open
wants to merge 3 commits into
base: 8.3
Choose a base branch
from

Conversation

dlubitz
Copy link
Contributor

@dlubitz dlubitz commented Oct 9, 2024

The creationDateTime has no setters in AbstractNodeData, so the NodeData::similarize can't set them in the target node propery. We need to allow the ObjectAccess::setProperty to force direct access to the class properties.

The lastModificationDateTime was also not copied before this bugfix and shouldn't be copied anyways.

Fixes: #5280

@dlubitz dlubitz self-assigned this Oct 9, 2024
@dlubitz dlubitz force-pushed the bugfix/5280-similarize-on-node-materialize branch from e350a91 to 95f164c Compare October 9, 2024 15:35
@dlubitz dlubitz force-pushed the bugfix/5280-similarize-on-node-materialize branch from 95f164c to 04763af Compare October 9, 2024 15:35
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.

+1 by reading

@dlubitz
Copy link
Contributor Author

dlubitz commented Oct 9, 2024

Unfortunately we can't "force direct access" to all properties, as this would skip the usage of the setters for all properties. So we need to distinguish the properties and the way we need to set them.

Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

if i would care for the old nodeData id say do it in line 769 via

$this->creationDateTime = $sourceNode->creationDateTime;

as this should(tm) work due to the scope being inside in one instance, but its time for neos 9 *g

@dlubitz
Copy link
Contributor Author

dlubitz commented Oct 10, 2024

if i would care for the old nodeData id say do it in line 769 via

$this->creationDateTime = $sourceNode->creationDateTime;

as this should(tm) work due to the scope being inside in one instance, but its time for neos 9 *g

You are absolutely right. But this is also true for all other properties. IDK why this was implemented that way. I didn't want to change that much, as this is just a bugfix.

@dlubitz
Copy link
Contributor Author

dlubitz commented Oct 10, 2024

Now it works as the code suggests. BUT if you do any change to materialize the node, this lastModificationDateTime should be now, right?

Due to this fix, the lastModificationDateTime stays the same as in the original node on the first change and gets updated on every following change. From my POV, this is wrong and we need to remove the lastModificationDateTime here in the similarize.

WDYT?

@mhsdesign
Copy link
Member

I would maybe look in how Neos 9 does it? timestamps->lastModified says Date and time a node was last modified in its content stream ... but i dont know how it behaves exactly.

@dlubitz
Copy link
Contributor Author

dlubitz commented Oct 11, 2024

I changed the bugfix, so we just copy the creationDateTime.

I assume we don't need to copy the lastModificationDateTime as it always make sence to have the current time as lastModificationDateTimeafter similarize a node. Moreover the code wasn't working anyways... so not change at all 😬

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.

3 participants