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

!!! Make Value Objects stricter #4133

Closed
bwaidelich opened this issue Mar 20, 2023 · 4 comments
Closed

!!! Make Value Objects stricter #4133

bwaidelich opened this issue Mar 20, 2023 · 4 comments
Labels
Milestone

Comments

@bwaidelich
Copy link
Member

bwaidelich commented Mar 20, 2023

We use a lot of Value Objects in the code for 9.0 but most of them lack validation.

IMO we should introduce some validation before a first release because that's impossible to do in a backwards--compatible manner lateron (it will always be possible to relax the rules without breaking changes).

We should also get rid of the magic __toString() methods from Value Objects in order to prevent accidental type conversion.

Example:

final class PropertyName implements \JsonSerializable
{
    private function __construct(
        public readonly string $value
    ) {
        if ($value === '') {
            throw new \InvalidArgumentException('Property name must not be empty.', 1519745994);
        }
    }
    // ...

    public function __toString(): string
    {
        return $this->value;
    }
}

could become sth like

final class PropertyName implements \JsonSerializable
{
    private const PATTERN = '/\w{1,100}/';

    private function __construct(
        public readonly string $value
    ) {
        if (preg_match(self::PATTERN, $value) !== 1) {
            throw new \InvalidArgumentException(sprintf('"%s" is not a valid Property Name because it does not match the regex pattern: %s", $value, self::PATTERN), 1519745994);
        }
    }
    // ...
}
@bwaidelich bwaidelich converted this from a draft issue Mar 20, 2023
@bwaidelich bwaidelich added the 9.0 label Mar 20, 2023
@skurfuerst skurfuerst added this to the 9.0 (ES CR) milestone Mar 20, 2023
@skurfuerst skurfuerst changed the title Make Value Objects stricter !!! Make Value Objects stricter Mar 31, 2023
@bwaidelich bwaidelich self-assigned this Apr 1, 2023
@bwaidelich bwaidelich moved this from Prioritized to In Progress in Neos 9.0 Release Board Apr 1, 2023
@bwaidelich bwaidelich moved this from In Progress 🚧 to Under Review 👀 in Neos 9.0 Release Board Apr 5, 2023
@github-project-automation github-project-automation bot moved this from Under Review 👀 to Done ✅ in Neos 9.0 Release Board Apr 14, 2023
@bwaidelich
Copy link
Member Author

Reopening, because #4156 only addresses the __toString() part of this, but I would like to go over the validation rules still

@bwaidelich bwaidelich reopened this Apr 17, 2023
@bwaidelich bwaidelich moved this from Done ✅ to In Progress 🚧 in Neos 9.0 Release Board Apr 17, 2023
@skurfuerst
Copy link
Member

We decided that:

  • NodeAggregateId should be 64 chars long max
  • ContentStreamId should be 40 chars long max

@skurfuerst
Copy link
Member

I think #4241 solves most things, but I am not sure if this is completely fixed already. @bwaidelich can look over this when he's back :)

@dlubitz dlubitz removed their assignment May 10, 2023
@bwaidelich
Copy link
Member Author

IMO this is done \o/

@github-project-automation github-project-automation bot moved this from In Progress 🚧 to Done ✅ in Neos 9.0 Release Board Sep 19, 2023
mhsdesign added a commit that referenced this issue Sep 20, 2023
mhsdesign added a commit that referenced this issue Sep 20, 2023
neos-bot pushed a commit to neos/neos that referenced this issue Oct 25, 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