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

!!! Remove shallow methods from Node? #4483

Closed
mhsdesign opened this issue Sep 4, 2023 · 3 comments
Closed

!!! Remove shallow methods from Node? #4483

mhsdesign opened this issue Sep 4, 2023 · 3 comments
Labels

Comments

@mhsdesign
Copy link
Member

mhsdesign commented Sep 4, 2023

Those three methods are super shallow and dont provide any additional information.

public function getProperty(string $propertyName): mixed;
public function hasProperty(string $propertyName): bool;
public function getLabel(): string;

i just want to open the discussion to make the Node even cleaner ;)

public function getProperty(string $propertyName): mixed
{
return $this->properties[$propertyName];
}
/**
* If this node has a property with the given name. Does NOT check the NodeType; but checks
* for a non-NULL property value.
*
* @param string $propertyName
* @return boolean
* @api
*/
public function hasProperty(string $propertyName): bool
{
return $this->properties->offsetExists($propertyName);
}
/**
* Returns the node label as generated by the configured node label generator
*
* @return string
*/
public function getLabel(): string
{
return $this->nodeType->getNodeLabelGenerator()->getLabel($this);
}

about the label it should imo be a constructor property given from the nodeFactory

@skurfuerst
Copy link
Member

I am unsure about this, because people rely on this right now very heavily. It's of course debatable whether this should be changed, but I right now am unsure about this because of backwards breakiness...

@kdambekalns
Copy link
Member

What would the replacement be? Using $properties and go from there? Sounds as if a few months from now someone will open a PR to add hasProperty() and getProperty()… 🙃

@mhsdesign
Copy link
Member Author

mhsdesign commented Sep 5, 2023

today we discussed this issue and wether to remove the array access from properties:

Why do our properties actually need to implement \ArrayAccess this is totally useless for php land - and out of sight of the content repository core there is no interest if something like a expression language like eel "requires" it to do funny stuff (see below) - i mean we also dont randomly implement the ProtectedContextAwareInterface because it might make things easier??

<?php

$node->getProperty("foo");

// vs

$node->properties["foo"];

(and eel)

${node.properties.bla}

# vs

${q(node).property("bla")}

imo - if we want this behavior, it should be implemented in eel, but please lets spare the cr xD


the conclusion of our weekly and this discussion is, that we want to keep the ArrayAccess behavior of the properties as this ensures a better backwards compatibility. If we would have started out from a fresh state we would certainly rethink all the ways but currently we will keep all of the property accessors

  • ${q(node).property("foo")}
  • ${node.properties.foo}
  • $node->getProperty(“foo”)
  • $node->properties["foo"]
  • $node->properties->offsetGet("foo")

@github-project-automation github-project-automation bot moved this from Todo to Done ✅ in Neos 9.0 Release Board Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

No branches or pull requests

3 participants