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

Add routable prefix to methods of RoutableInterface #155

Conversation

niklasnatter
Copy link
Contributor

I think we should prefix the methods (and therefore the keys of the normalized array) for all behaviours included in the bundle. The RoutableInterface is the only interface that did not use a prefix for its methods.

I see the following advantages:

  • This prevents naming conflicts (method names and also keys in the normalized array)
  • The getIgnoredAttributes method of the Normalizer classs are specific. At the moment, a project cannot normaliza a contentId because the RoutableNormalizer prevents it

@@ -18,12 +18,12 @@
*/
interface RoutableInterface
{
public function getResourceKey(): string;
public function getRoutableResourceKey(): string;
Copy link
Contributor Author

@niklasnatter niklasnatter Jun 4, 2020

Choose a reason for hiding this comment

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

The getResourceKey method shoukd be on the same entity as the getTemplateType and getWorkflowName IMO.

Therefore I am thinking about moving the getResourceKey method from the ContentRichEntityInterface to the DimensionContentInterface in another pull request. What do you think?

@@ -14,7 +14,6 @@
"locale": "en",
"published": null,
"publishedState": false,
"resourceKey": "@string@",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added with #154 which was merged just an hour ago. So i think it should not be a big BC break 🙂

@@ -100,9 +100,4 @@ public static function getTemplateType(): string
{
return Example::TEMPLATE_TYPE;
}

public static function getContentClass(): string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we forgot about this in #154

*
* @see MockWrapper to learn why this trait is needed.
*/
trait ContentRichEntityMockWrapperTrait
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this wozld not be necessary if we move the getResourceKey method

Copy link
Member

@alexander-schranz alexander-schranz left a comment

Choose a reason for hiding this comment

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

I'm definitly agains this change. We should not prefix this methods. Else he will need to implement getTemplateLocale, getRouteLocale, ... we should avoid the prefix. Then again the same for getTemplateResourceKey, getRouteableResourceKey.

Definitly a no go from my side. Sorry 👎

@niklasnatter
Copy link
Contributor Author

niklasnatter commented Jun 4, 2020

Not sure about that - if two behaviour interfaces (eg TemplateInterface and RoutableInterface) have a getResourceKey method, we cannot add the getResourceKey method to both traits because they would conflict. Maybe these unprefixed methods (resource key, locale, ...) should be on the DimensionContentInterface?

But i think that is a discussion for another day 🙂 I adjusted this PR to only rename the getContentId to getRoutableId. Would like to get rid of the contentId term because we dont use that anywhere else and it might be confusing (already saw that developers used it outside of the context of routes).

@alexander-schranz
Copy link
Member

The issue we have with serialization is what we have documented #113 that we should define what should be serialized and not what should not be serialized. Not 100% happy with getRoutableId but we could go with that currently.

@niklasnatter
Copy link
Contributor Author

Do you have a better idea than getRoutableId? 🙂

@alexander-schranz alexander-schranz merged commit 667b4a7 into sulu:master Jun 4, 2020
@alexander-schranz
Copy link
Member

@nnatter currently not 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants