!!! TASK: Remove magic __toString methods from Value Objects

[bwaidelich]

Removes the magic __toString() method from Value Objects of the Neos.ContentRepository.Core package and adjusts the code accordingly.

This will hopefully make the code more robust because it prevents accidental type conversions and allows static type analyzers and IDEs to detect bugs more easily.

For the following string based VOs a corresponding public readonly value property exists:

• ContentDimensionId
• ContentDimensionValue
• ContentRepositoryId
• PropertyType
• NodeType
• NodeTypeName
• NodePath
• NodeAggregateId
• NodeName
• PropertyName
• ReferenceName
• UserId
• ContentStreamId
• WorkspaceDescription
• WorkspaceName
• WorkspaceTitle
• ContentStreamEventStreamName (internal)
• ContentSubgraphVariationWeight (internal)
• NodeRelationAnchorPoint (internal)
• ContentDimensionValueSpecializationDepth (internal)

For these set-VOs, a toJson() method was added:

• DimensionSpacePoint
• OriginDimensionSpacePoint
• DimensionSpacePointSet
• OriginDimensionSpacePointSet
• ParentNodeMoveDestination
• SucceedingSiblingNodeMoveDestination
• NodeAggregateIds
• CoverageByOrigin (internal)
• OriginByCoverage (internal)

For NodeTypeConstraints and NodeTypeConstraintsWithSubNodeTypes a toFilterString() method was added.
The value of SerializedPropertyValue

Breaking Change

This is a breaking change because it requires adjustments to code that implicitly or explicitly casts the affected types to string.

Examples

The following examples have to be replaced...

'some' . $contentStreamId;
sprintf('Node ID: %s', $nodeAggregateId);
$some->methodExpectingAString($workspaceName);
echo $dimensionSpacePointSet;

...with:

'some' . $contentStreamId->value;
sprintf('Node ID: %s', $nodeAggregateId->value);
$some->methodExpectingAString($workspaceName->value);
echo $dimensionSpacePointSet->toJson();

Fusion

In Fusion code, the same is true, so the following has to be replaced...

${q(node).get(0).name}
{node.nodeAggregateId + 'suffix'}

...with:

${q(node).get(0).name.value}
{node.nodeAggregateId.value + 'suffix'}

Fluid

Likewise, in Fluid templates the following has to be replaced...

<f:link.action action="show" arguments="{workspace: workspace.workspaceName}">Link</f:link.action>
{node.name -> f:format.stripTags()}
<p>{node.nodeAggregateId}</p>

...with:

<f:link.action action="show" arguments="{workspace: workspace.workspaceName.value}">Link</f:link.action>
{node.name.value}
<p>{node.nodeAggregateId.value}</p>

Related: #4133

[ahaeslich]

As already stated in slack:

Merged the last changes from v9 branch and tested it with my v9 dev setup using the refactored Neos.Demo.
Pushed some fixes for bugs that I found.

[kitsunet]

Despite it being merged, I would like to argue with that with strict typing no accidental type casting happens and it's not really magic to implement a language provided method that allows casting in a defined way when needed. This looks like a big PITA in fusion to me (and I don't really see the big pro for PHP either), especially remembering where to do it. I am not a huge fan.

[bwaidelich]

Despite it being merged

We're aware that this is a change that will lead to friction and we can always revert if it turns out not to be worth it.

with strict typing no accidental type casting happens

I beg to differ and I actually found quite some bugs by making type conversions explicit. So even if we were to revert it, we would profit from the intermediate change.

it's not really magic to implement a language provided method that allows casting in a defined way when needed

__toString() is a magic method by definition ;)
IMO we should never rely on the implicit string casting of objects because that also violates semantics. Eg. DimensionSpacePoint::__toString() JSON-encoded the values which can be unexpected at best and unstable/slow at worst.
Besides, some (string)$object might return a human readable representation of the VO for debugging/outputting – that's why we decided to completely remove it because that will at least make you aware of this.

Marco Pivetta summarized his reasoning in a Github comment

This looks like a big PITA in fusion to me

The Neos.Demo site didn't require adjustments AFAIR. But I agree that Fusion and Fluid will break in a lot of cases.
Some of which we can address automatically via neos/rector#11 (and follow-ups).
For the rest I think we should adjust our prototypes, EEL Helpers (and Fluid ViewHelpers) to be good citizens:

function someHelper(NodeAggregateId|string|null $nodeAggregateId) {
    if (is_string($nodeAggregateId)) {
        $nodeAggregateId = NodeAggregateId::fromString($nodeAggregateId);
    }
    // ...
}

[kitsunet]

I beg to differ and I actually found quite some bugs by making type conversions explicit.

Just to make sure we talk about the same thing (string)$identifier is explicit conversion to me, or do you mean something else? I expect that with strict types a foo(string $bar) method only accepts foo((string)$identifier) but not just foo($identifier), just like '123' === (string)$identifier. or were do we talk past each other here?

__toString() is a magic method by definition ;)

Fair I guess I set myself up here, but I am not sure I would call that magic in the way AOP is for example, it's a language feature that allows you to cast something, I think it should and can be used, in fact I would be happy to have more of that magic in the language, comparison operator behavior for example woudl be neat, but yes, it's a magic method, in the way that it has special meaning in the language, it's pretty transparent to every tool though, from IDEs to static analyzers.

I agree though that implicit casting is a problem, also that we use it for different things (JSON, collections) I agree is problematic, I mostly see it for "simple" string wrapping DTOs, like identifiers.

I can follow ocramius argument only to an extend, I see what he is saying, but the first example was an implementation decision, I am casting a variable that can or cannot be the thing, not specifically an object with a __toString(), if the string cast was on the DTO itself, the problem woudln't exist, and in the second example, well the signature changed, either way the consumers need to check this. and if I need a string in the first method, I have no choice but to convert the incoming null to one either way, so IMHO both are not super convincing. Yet I get it, for PHP I don't see it as a big deal either way and am totally on board with value. IDE features will help me with that anyways.

For Fusion I rather meant the inverse direction, not in the helper (although those ifs are annoying), but rather having to remember something like node.nodeAggregateId.value (here btw. also the "Id" comes around as you need to remember to use the abbreviation) instead of node.identifier. Less IDE/completion support there, so trickier to get help for and I imagine the errors down the line if you actually used eg. node.nodeAggregateId might not be helpful either to get you on the right track what to do.

[kitsunet]

I forgot a conclusion...

The last paragraph above bothers me, but on the PHP side I am pretty fine with this. Who knows what the right move is, but I like to err on the side of implementers having a good time.

[bwaidelich]

@kitsunet Thanks for your elaborate response!

Just to make sure we talk about the same thing (string)$identifier is explicit conversion to me, or do you mean something else?

Yes, it is explicitly converting the type. But it is a little implicit in the meaning.
Eg. some VOs returned a JSON representation, some the internal value and some a label. Corresponding toJson(), label() and value properties/functions are more specific.

But I'm more concerned about implicit casts like
'some-string' . $someVo or sprintf('foo %s', $someVo)

I expect that with strict types a foo(string $bar) method only accepts foo((string)$identifier)

Unfortunately that's only true if the calling side uses strict mode, too – I had to learn. See #4094 (comment)

The last paragraph above bothers me

Unfortunately that's the one I didn't fully grasp :)

But I can phrase my concerns and you can check if they overlap or whether I'm missing something:
In PHP the change is breaking, but at least very obvious and easy to fix.
In Fusion a node.identifier will return null by default, not leading to any errors.
We have rector to detect (most of) these cases but they still remain.

I hope that we can cater for the remaining cases if we know them better.

For example:

<f:link.action action="show" arguments="{workspace: workspace.workspaceName}">...

is nasty for example, because it leads to a confusing exception message of

Tried to convert an object of type "Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName" to an identity array, but it is unknown to the Persistence Manager.

But maybe we can improve on that?

Apart from that I wonder where we actually need node.nodeAggregateId.value in Fusion (or Fluid).

[ahaeslich]

Apart from that I wonder where we actually need node.nodeAggregateId.value in Fusion (or Fluid).

For this one I created an issue with a short example: neos/rector#12

[bwaidelich]

For the record: I also think that node.nodeAggregateId.value is awkward in Fusion and should be replaced by node where possible.
E.g. for the mentioned example of making the node id an attribute of the content element (in order to create an anchor link) we could introduce a dedicated prototype

@process.augment = Neos.Fusion:Augmenter {
  id = ${node.nodeAggregateId.value}
}

=>

@process.addAnchorLink = Neos.Neos:NodeAnchorLink {
  node = ${node}
  prefix = 'anchor-'
}

(or a better name)

[kitsunet]

Thx! Ok lets split this apart, I think I muddled the last paragraph a bit together but I think we are in agreement :)

function someHelper(NodeAggregateId|string|null $nodeAggregateId) {
    if (is_string($nodeAggregateId)) {
        $nodeAggregateId = NodeAggregateId::fromString($nodeAggregateId);
    }

Kinda fugly if you ask me, but I guess no way we can fix that. I think I would prefer in the future (therefore also my comments on the AsssetHelper #4155 )

function someHelper(?NodeAggregateId $nodeAggregateId);
function someHelperFromString(?string $nodeAggregateId);

# or with more meaningful names

function nodeByAggregateId(?NodeAggregateId $nodeAggregateId);
function nodeByIdString(?string $nodeAggregateId);

Still not a perfect example but I guess you get the idea.

Then another topic, the decision to shorten Identifier, we have always abstained from using shorts and I think funny enough I was in the meeting when you talked about usign id because the names are already so long and it's obvious what it means and I kinda agreed, and with modern IDEs and tooling I think there is no huge case against abbreviations etc. in names anymore, BUT in fusion if you don't have that perfect auto complete, just knowing the rule is no abbreviations did help (me) a lot. So here that decision is falling on our feet a bit IMHO. I personally would happily write the additional characters in fusion to have nodeAggregateIdentifier if that means I can not think about abbreviations at all. But too late to change now I guess.

Finally the general usage in fusion, I guess what you write is the way to go, I personally used node.identifier quite a bit, so the change is painful to me but presenting users with nice alternatives might actually be an improvement over the status quo, so to look forward instead of backward, I guess I will dedicate myself to try and add these kind of helpers and prototypes to help users in the future as well as look at possible improvements to errors that might occur ❤️

[bwaidelich]

Kinda fugly if you ask me

IMO EEL helpers are the Anti corruption layer between the highlevel integrator world of fusion and the (hopefully) type safe world of PHP. So IMO it's totally OK to have some glue code there.

And for the same reason I would prefer a Fusion code like

${Some.helper(node)} // or ${Some.helper(node.nodeAggregateId)}

over

${Some.helperFromString(node.nodeAggregateId)}

Then another topic, the decision to shorten Identifier, we have always abstained from using shorts

We always made exceptions to the rule ("HTML", "YAML", "PHP" to name a few) and discussed this quite a while ago.

I see your point though. And if it was node.identifier => node.id even more – but since it changes anyways, I don't see why nodeAggregateIdentifier is much better than nodeAggregateId.

I personally used node.identifier quite a bit

We could maybe add a getIdentifier() method to the Node read model to relief the pain!?
Or even add some compatibility layer within Fusion (@skurfuerst I think, you mentioned that before?)

Nevertheless, I would like to collect those cases to see if we can solve them differently in order to stay on a higher level (like with the anchor example)

[kitsunet]

IMO EEL helpers are the Anti corruption layer between the highlevel integrator world of fusion and the (hopefully) type safe world of PHP. So IMO it's totally OK to have some glue code there.

Yep, yep, this was mostly a longer explanation of what I meant, and I am fine with this, you are right, it IS an anti corruption layer and thus okay to be not beautiful.

Then another topic, the decision to shorten Identifier, we have always abstained from using shorts

We always made exceptions to the rule ("HTML", "YAML", "PHP" to name a few) and discussed this quite a while ago.

Note that all those examples are acronyms, so doing it for single words is new. And yes, I agree we do for those and such extending it to id can be fine, I still find it not ideal in terms of consistency, and we should make sure that we don't start doing it more IMHO. :)

I see your point though. And if it was node.identifier => node.id even more – but since it changes anyways, I don't see why nodeAggregateIdentifier is much better than nodeAggregateId.

It's just more consistent I guess? but again, no big deal. WAs again just more clarification.

I personally used node.identifier quite a bit

We could maybe add a getIdentifier() method to the Node read model to relief the pain!? Or even add some compatibility layer within Fusion (@skurfuerst I think, you mentioned that before?)

Nevertheless, I would like to collect those cases to see if we can solve them differently in order to stay on a higher level (like with the anchor example)

Yep ,as said, I would look into that and try to find ways to make it easier for users. <3