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

Adjust interface of services to accept locale instead of dimensionAttributes array #84

Closed
alexander-schranz opened this issue Feb 21, 2020 · 1 comment
Labels
Technical Debt Impacts only code quality, no or just small impact on end developers and users To Discuss The core team has to decide if this will be implemented

Comments

@alexander-schranz
Copy link
Member

alexander-schranz commented Feb 21, 2020

This issue relates to #87.

Right now, most of the public services of the bundle (ContentPersister, ContentWorkflow, ContentResolver) accept a dimensionAttributes array. This is needed for flexibility in case somebody adds an additional attribute to the Dimension entity. But it also causes some inconsistencies and makes the interface of this services harder to understand/error prone.

For example, we should always change the DimensionContent of the draft stage when modifying/persisting new content. Unfortunately, right now it is possible to call the ContentPersister with a dimensionAttributes array that contains ['stage' => 'live']. This is probably not desired as it would modify the content of the website but the admin would still display the old content.

If we decide to allow only a stage and a locale attribute, we could remove the dimensionAttributes parameter from the public interface of our services. This would make the services more concise and easier to use.

@alexander-schranz alexander-schranz added the Technical Debt Impacts only code quality, no or just small impact on end developers and users label Feb 21, 2020
@niklasnatter niklasnatter changed the title Change modify handlers to accept only locale Adjust interface of services to accept locale instead of dimensionAttributes array Feb 24, 2020
@niklasnatter niklasnatter added the To Discuss The core team has to decide if this will be implemented label Feb 25, 2020
@niklasnatter
Copy link
Contributor

After using the bundle in different projects, I changed my mind on this. During development I quickly got used to the dimensionAttributes parameter and did not think about it a lot after that. I still think removing the dimensionAttributes parameter would make our interfaces simpler, but it would also remove a lot of flexibility. For example, it would not be possible to implement something like the getDefaultAttributes method that was added in https://github.com/sulu/SuluContentBundle/pull/174/files#diff-1edf6cffae3d6fc5c10993e29318d205d1a7fb67f3ae6ada8d97a7a61efcf828R40

Therefore, instead of removing the dimensionAttributes parameter, we should make sure to use and handle it consistently in our services. If we do that, I am quite sure our users will quickly get used to the concept 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Technical Debt Impacts only code quality, no or just small impact on end developers and users To Discuss The core team has to decide if this will be implemented
Projects
None yet
Development

No branches or pull requests

2 participants