neos / neos-development-collection

The unified repository containing the Neos core packages, used for Neos development.
https://www.neos.io/
GNU General Public License v3.0
260 stars 220 forks source link

BUGFIX Workspace aware NodeCacheEntryIdentifier #5083

Closed dlubitz closed 2 weeks ago

mhsdesign commented 1 month ago

Im currently digging into the history of this NodeCacheEntryIdentifier what is it and since when? Its def new with. 9.0

bwaidelich commented 1 month ago

Nice. Such a simple change. I would be much more confident if we had a test for this, preferably a behat test. But I'm not sure how that would look like?!

mhsdesign commented 1 month ago

originally the Neos 8.3 node was a CacheAwareInterface:

https://github.com/neos/neos-development-collection/blob/eada1dbfb569de6e3932b26b4b3c630401020e05/Neos.ContentRepository/Classes/Domain/Model/Node.php#L1806

With this pr https://github.com/neos/neos-development-collection/commit/8336726355e6f70e5a71b8df5df8cf7396c03c55 the method was removed from the Neos 9-dev Node (in preparation for standalone).

A few days later Neos.Caching.entryIdentifierForNode was introduced https://github.com/neos/neos-development-collection/commit/62555a1ab4eb992dee5bda9b4a778c7ac763062c

And code was rewritten to use this new helper

  @cache {
     mode = 'cached'
     entryIdentifier {
-       documentNode = ${node}
+       documentNode = ${Neos.Caching.entryIdentifierForNode(node)}
mhsdesign commented 1 month ago

While on it i would also like to move it to Neos\Neos\Fusion\Cache as well? :D (but could do so as well :D)

mhsdesign commented 1 month ago

I know this pr is kindof exploding with comments relative to the lines changed but imo thats a good part. For example until now i was not really aware of this breaking change from 8.3 and so with this in mind we could possibly back-port Neos.Caching.entryIdentifierForNode to make live easier ;)

Also to better fit the fusion pattern: being rather short we could also consider using Neos.Caching.entryIdentifier(node) or implement magic so people wont have to use this helper at all? Or implement a flowquery.

Imo fusion is the part were we should put much dedication into as well, that means knowing all breaking changes and designing a well suited api. (The original change was obscured in a 148 additions and 71 deletions change set which would not even make it to auto generated release notes)