neos-development-collection icon indicating copy to clipboard operation
neos-development-collection copied to clipboard

!!! TASK: Remove magic underscore access for internal node properties

Open mhsdesign opened this issue 2 years ago • 7 comments

Resolves: https://github.com/neos/neos-development-collection/issues/4208 Related: https://github.com/neos/neos-development-collection/pull/4921

We have the _* underscore access syntax to access actual php object properties of the Node. Due to the ESCR rewrite the Node object looks completely different and has way less getters / properties. These are the new properties:

  • _aggregateId, _originDimensionSpacePoint, _classification, _nodeTypeName, _name, _timestamps

While these are the ones known from 8.3

  • _nodeType (removed via https://github.com/neos/neos-development-collection/issues/5019)
  • _properties (useless)

The actual important and ones used in 8.3 like _identifier _depth _parent _hiddenAfterDateTime _hiddenBeforeDateTime etc (see https://github.com/neos/neos-development-collection/issues/4208#issuecomment-1573345069) will not work and have to migrated with rector.

To avoid confusion about which magic underscore properties still exist in 9.0 a simple way is - it thought we agreed upon this in the meeting from the 19.01 link - to NOT have any underscore property logic and remove any magic from this syntax. Like one can use an underscore now in consumer land but it is never any more special than any other character.

Thats why this pr remove partially implemented legacy support for _identifier as well as _depth as we have rector migrations for this to properly use the respective eel helper or eel property access directly.

There is a little but as discussed in on the 26.04 we concluded to still allow sorting nodes by date via the q().sort() we will likely have to make one exception regarding underscore properties: The sort operation will translate _lastModificationDateTime and respective things to the 9.0 equivalent logic internally instead of having to introduce another sort operation or a special parameter or another special syntax like .sort(['$$lastModified']) or such.

Syntax 8.3 getter and documentation 9.0 proposed getter and documentation
_creationDateTime getCreationDateTime() timestamps->created Date and time a node was created in its content stream
_lastModificationDateTime getLastModificationDateTime() timestamps->lastModified Date and time a node was last modified in its content stream
_lastPublicationDateTime getLastPublicationDateTime() Date of last publication or null if the node was not published yet timestamps->originalLastModified Date and time a node was last modified in its original content stream

Upgrade instructions

Review instructions

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

mhsdesign avatar Apr 26 '24 08:04 mhsdesign

one todo is: how to sort by creation date in Neos 9?? Something like node.timestamps.originalCreated https://github.com/neos/neos-development-collection/issues/4208#issuecomment-1986006454

in our weekly from the 26. we discussed that instead of adding a new flowquery or a magic parameter, we probably want to support the old legacy 8.3 syntax _creationDateTime or _lastModificationDateTime or even _lastPublicationDateTime.

The question is if these modifiers should only work for the sort operation or also for property and filter as well as part of a fizzle query [_creationDateTime = 123]. (Fizzle is almost impossible to archive with the optimisation https://github.com/neos/neos-development-collection/pull/4712 in place)

mhsdesign avatar May 04 '24 11:05 mhsdesign

todos

  • [x] as we relocate the sort operation to the other flow-queries and touch its logic i seem to have to carry out the burden to migrate the old functional test https://github.com/neos/neos-development-collection/blob/fe0063b7e4fbaea9b0a72abfd323354fabf42173/Neos.Neos/Tests/Functional/FlowQueryOperations/SortOperationTest.php to behat
  • [x] rector migration for node.hidden and _hidden to Neos.Node.isDisabled(node) https://github.com/neos/rector/pull/83

mhsdesign avatar May 09 '24 08:05 mhsdesign

I do not understand why you did this?

kitsunet avatar May 09 '24 09:05 kitsunet

That was what i have been discussing in https://github.com/neos/neos-development-collection/issues/4208 or in the meetings i thought but it seems we have talked past each other?

My thesis in short (now part of the main description):

We have the _* underscore access syntax to access actual php object properties of the Node. Due to the ESCR rewrite the Node object looks completely different and has way less getters / properties. These are the new properties:

  • _subgraphIdentity
  • _nodeAggregateId
  • _originDimensionSpacePoint
  • _classification
  • _nodeTypeName
  • _nodeName
  • _timestamps

While these are the ones known from 8.3

  • _nodeType (until https://github.com/neos/neos-development-collection/issues/5019)
  • _properties (useless)

The actual important and ones used in 8.3 like _identifier _depth _parent _hiddenAfterDateTime _hiddenBeforeDateTime etc (see https://github.com/neos/neos-development-collection/issues/4208#issuecomment-1573345069) will not work and have to migrated with rector.

To avoid confusion about which magic underscore properties still exist in 9.0 a simple way is - it thought we agreed upon this in the meeting from the 19.01 link - to NOT have any underscore property logic and remove any magic from this syntax. Like one can use an underscore now in consumer land but it is never any more special than any other character.

Thats why this pr remove partially implemented legacy support for _identifier as well as _depth as we have rector migrations for this to properly use the respective eel helper or eel property access directly.

There is a little but as discussed in on the 26.04 we concluded to still allow sorting nodes by date via the q().sort() we will likely have to make one exception regarding underscore properties: The sort operation will translate _lastPublicationDateTime and respective things to the 9.0 equivalent logic internally instead of having to introduce another sort operation or a special parameter or another special syntax like .sort(['$$lastModified']) or such.

mhsdesign avatar May 09 '24 11:05 mhsdesign

This is all clear, but

Extracted everything i though this pr was originally meant to be into https://github.com/neos/neos-development-collection/pull/5015 and added the things i found missing.

why, why didn't you just add the things missing to my original PR?

kitsunet avatar May 09 '24 11:05 kitsunet

Because i thought it would get too complex and the _hiddenInIndex thing is somehow related but a different task and should imo deserve its own pr. (Also i asked you in slack if im allowed to do this :D)

mhsdesign avatar May 09 '24 11:05 mhsdesign

Also i asked you in slack if im allowed to do this :D

Too much going on LOL, seems still a bit weird but 🤷

kitsunet avatar May 09 '24 11:05 kitsunet

The migration to use the herby introduced Neos.Node.isDisabled(node) helper can be reviewed here: https://github.com/neos/rector/pull/83

This pr is ready for final reviews.

If i get feedback regarding "9.0 proposed getter and documentation" from above i can also add that documentation to the node.

mhsdesign avatar Sep 24 '24 20:09 mhsdesign