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

EZP-31647: more flexibility added to AttributeMapper::supports #152

Open
wants to merge 1 commit into
base: 2.0
Choose a base branch
from

Conversation

SerheyDolgushev
Copy link
Contributor

Question Answer
JIRA issue EZP-31647
Bug/Improvement yes
New feature no
Target version 2.0
BC breaks no
Tests pass yes
Doc needed yes

Before this PR EzSystems\EzPlatformRichText\Configuration\UI\Mapper\CustomTag\AttributeMapper::supports was accepting only $attributeName. But in some cases, it is vital to have attribute mappers for specific custom tags/attributes.

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@dew326 dew326 requested a review from alongosz May 22, 2020 06:08
@SerheyDolgushev
Copy link
Contributor Author

@alongosz can you please review this one?

@lserwatka lserwatka requested a review from a team September 23, 2020 07:15
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.

I'm sorry, but I'm totally not following what you want here.
Are you requesting a feature or fixing a bug?

AttributeMapper is internal, thus there's no BC promise on that. It also implies that anything outside of its current function is not supported.
What do you need exactly?

@SerheyDolgushev
Copy link
Contributor Author

It is a feature request.

Let's say there are two custom tags with my-choice choice attribute:

  1. Custom tag tag-A where the list of possible options for my-choice attribute is static
  2. Custom tag tag-B where the list of possible options for my-choice attribute is dynamic, let's say it is fetched from some 3rd party API.

Right now it is possible to have an attribute mapper for all the choice custom tag attributes. But it is impossible to have an attribute mapper for just a specific attribute of the specific custom tag. This PR provides this feature and will let to have an attribute mapper only for the my-choice attribute of tag-B custom tag.

@alongosz please let me know if you have any questions.

@alongosz
Copy link
Member

Hmm, this feature (as always) can go in master only. We also need to decide then to expose AttributeMapper as a strict contract, to be discussed with @ezsystems/php-dev-team how/where. Not sure when this is gonna happen, because we're all quite busy now and API extension point design is not an easy thing to assess. I'm also not 100% sure it's not gonna mess up existing code. You can reach out to @SylvainGuittard to see if he can prioritize this.

For now what is required are some configuration examples and unit test coverage.

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

Successfully merging this pull request may close these issues.

2 participants