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

EZEE-3085: Added specification ContentTypeSpecification #47

Merged

Conversation

michal-myszka
Copy link
Contributor

@michal-myszka michal-myszka commented Apr 9, 2020

Question Answer
JIRA issue EZEE-3085
Type improvement
Target eZ Platform version v3.0.1 - please update accordingly
BC breaks no
Tests pass yes
Doc needed no

Added specification ContentTypeSpecification

Checklist:

  • PR description is updated.
  • Tests are implemented.
  • Added code follows Coding Standards (use $ composer fix-cs).
  • PR is ready for a review.

@michal-myszka michal-myszka requested review from Nattfarinn, alongosz, webhdx and a team April 9, 2020 13:05
@michal-myszka michal-myszka self-assigned this Apr 9, 2020
Copy link
Contributor

@Nattfarinn Nattfarinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide unit tests for ContentTypeSpecification.

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate on the purpose of this and naming?
I don't see this doing anything right now (not configured?).

I see that naming was taken from SpecificationInterface defined by @mikadamczyk in AdminUI in 2018.
@mikadamczyk if you remember, could you explain the choice of naming? Is it some kind of a design pattern or does it reflect existing solution? I don't remember being part of this discussion.

Without the broader context I don't feel like this belongs to the Repository domain

@michal-myszka
Copy link
Contributor Author

Could you elaborate on the purpose of this and naming?
I don't see this doing anything right now (not configured?).

I see that naming was taken from SpecificationInterface defined by @mikadamczyk in AdminUI in 2018.
@mikadamczyk if you remember, could you explain the choice of naming? Is it some kind of a design pattern or does it reflect existing solution? I don't remember being part of this discussion.

Without the broader context I don't feel like this belongs to the Repository domain

This becomes from Behavioral Design Pattern - Specification:
https://designpatternsphp.readthedocs.io/en/latest/Behavioral/Specification/README.html

The ContentTypeSpecification is currently used in
https://github.com/ezsystems/ezplatform-page-fieldtype/pull/141/files#diff-5adf309520410df755badb9f39017b56R51
and should be available for the other packages.

@alongosz
Copy link
Member

alongosz commented Apr 9, 2020

This becomes from Behavioral Design Pattern - Specification:
https://designpatternsphp.readthedocs.io/en/latest/Behavioral/Specification/README.html

The ContentTypeSpecification is currently used in
https://github.com/ezsystems/ezplatform-page-fieldtype/pull/141/files#diff-5adf309520410df755badb9f39017b56R51
and should be available for the other packages.

Because it's used in the following way there

$contentTypeSpecification = new ContentTypeSpecification($expectedType);

the ContentTypeSpecification implementation should also be a part of SPI. We don't give BC promise on anything from Core, unless documented. Documented Core stuff is a technical debt at this point.
If you were to define ContentTypeSpecification in the Page Field Type package, then what we have, minus that implementation would be fine.
We could consider ContentTypeSpecification an API, but then it can't be "newable" externally.

BTW. Specification is a Value (if I were to suggest the placement) ;)

TBH it looks like an overkill, but as long as it follows A(S)PI rules, I'm fine ;)

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok for SPI,
Remarks for tests:

@michal-myszka
Copy link
Contributor Author

Merged up to master

@alongosz alongosz deleted the EZEE-3085-added-content-type-validation-for-page-fieldtypes branch June 27, 2022 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants