Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem: ExpressionContainerInterface::addExpressionObject() has a $return_uuid parameter. That is bad OOP practice because then the return value is inconsistent. A method should always return the same type of thing to not be confusing.
Proposed solution: create a new addExpressionObjectReturnUuid() method that returns the UUID, change addExpressionObject() to always return $this.
Comments
Comment #2
fagoAgreed on the bad practice and removing the parameter. I'm not sure about adding addExpressionObjectReturnUuid() though, that sounds a bit ugly also. Cannot we just ask the expression about its UUID after adding?
Comment #3
klausiI would like to avoid the UUID in the expression. The UUID is an identifier within the expression list of the container (within the action set for example). If we push down the UUID to the expression object we are "polluting" it with state information from the parent container, and I want to avoid that. That way an expression object is independent from its location in any container - it can be added multiple times in multiple places and will still work. If you add the same expression object to different containers the UUID in it will change and it gets confusing.
In the end it might not be possible/feasible to avoid the UUID in the expression object, but I would like to try as long as possible. It keeps a cleaner network of nested expressions.
Comment #4
fagoI'm not sure it's really "state" - the entity id is also not state of the entity, is it? :)
>It keeps a cleaner network of nested expressions.
I'm not sure it really is - now the parent has the information about IDs of its children. Wouldn't it be cleaner when the children know about their IDs and the parent don't? Why should someone else know the expressions ID and not the expression?
I think that's how it would have to function anyway. I don't see a use case for having the same expression tree "linked" two to places in a tree. If you add an expression two times, we'd have to deal with that and make sure we are cloning it the second time or we will run into issues at latest after serialization, where the "link" would get lost anyway.
So on the contrary, we'd need the UUID to take care of the case of adding the expression twice and clone it when it's added the second time - or throw an exception. I.e., if the expression has an uuid generated, we can assume it's already nested somewhere.
Comment #5
klausiHm ok, let me try UUIDs on expressions and see how that looks.
Comment #6
klausiPR: https://github.com/fago/rules/pull/377
Comment #7
fagoGreat! Merged and pushed!