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

Allow filter directive processors to be used with the Widget template filter #28723

Conversation

theCapypara
Copy link
Member

@theCapypara theCapypara commented Jun 15, 2020

Description (*)

This allows the use of filter directives with the Magento\Widget\Model\Template\Filter filter (which is used by CMS content). This makes extending the filter directives actually useful.

By applying the changes in this Pull Request, the following DI configuration also applies to this filter as well:

<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:ObjectManager/etc/config.xsd">
    <type name="Magento\Framework\Filter\Template">
        <arguments>
            <argument name="directiveProcessors" xsi:type="array">
                <item name="my-custom-directive" sortOrder="1000" xsi:type="object"><!--...--></item>
            </argument>
        </arguments>
    </type>
</config>

Manual testing scenarios (*)

  1. Create a custom filter directive via DI
  2. See this filter directive now being applied for CMS pages and CMS blocks

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

Resolved issues:

  1. resolves [Issue] Allow filter directive processors to be used with the Widget template filter #29764: Allow filter directive processors to be used with the Widget template filter

@m2-assistant
Copy link

m2-assistant bot commented Jun 15, 2020

Hi @Parakoopa. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

@theCapypara
Copy link
Member Author

It seems contributors can't add labels anymore. This is a partner contribution for "Tudock GmbH", could someone add the labels for that?

@theCapypara theCapypara changed the title Allow directiveProcessors to be used with Widget template filter Allow filter directive processors to be used with the Widget template filter Jun 15, 2020
@theCapypara
Copy link
Member Author

@magento run all tests

@theCapypara
Copy link
Member Author

@magento-engcom-team The failing builds are not because of changes I made, it's just something old in the same file, what can I do about this?

@VladimirZaets VladimirZaets added Priority: P3 May be fixed according to the position in the backlog. Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Jun 18, 2020
@lenaorobei
Copy link
Contributor

@magento create issue

Copy link
Contributor

@lenaorobei lenaorobei left a comment

Choose a reason for hiding this comment

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

Hi @Parakoopa. Thanks for this improvement.

Static builds work in a way that the whole content of changed file is being checked.

For this specific failure The use of function html_entity_decode() is discouraged: I believe it makes sense to add // phpcs:ignore Magento2.Functions.DiscouragedFunction annotation, since it is not related to your change.

Please consider also adding integration test to cover this behaviour.

@engcom-Charlie
Copy link
Contributor

Hi @Parakoopa could you please take a look at #28723 (review)?
Otherwise, we can't proceed with your PR.
Thank you.

@engcom-Charlie
Copy link
Contributor

@magento run all tests

@theCapypara
Copy link
Member Author

@engcom-Charlie I don't really understand why that test is failing now.

@lenaorobei
Copy link
Contributor

Static tests pass now, but this change still requires additional test coverage. I believe, integration test would suit best.

@engcom-Charlie
Copy link
Contributor

Hi @Parakoopa, can you please cover your changes by integration test?
Thank you.

@gabrieldagama
Copy link
Contributor

The risk was set tolow due to: the suggested changes shouldn't impact other areas.

@theCapypara
Copy link
Member Author

I will implement the integration tests as soon as I find the time for it.

@engcom-Charlie
Copy link
Contributor

Hi @Parakoopa, do you have any updates about test coverage?
Thank you.

@theCapypara
Copy link
Member Author

I still have this on my TODO list.

@theCapypara
Copy link
Member Author

@magento run all tests

@theCapypara
Copy link
Member Author

@engcom-Charlie
Is there any update on this? I did the requested changes.

@theCapypara
Copy link
Member Author

theCapypara commented Oct 25, 2021

Any update on this? I haven't heard anything in over a year.

@theCapypara
Copy link
Member Author

@magento-engcom-team Any updates?

@engcom-Charlie engcom-Charlie removed their assignment Jan 13, 2022
@engcom-Charlie
Copy link
Contributor

Hi @theCapypara,

Thank you for your contribution!

Once the PR will get reviewed from community members, it will move further.

Thank you!

@hostep
Copy link
Contributor

hostep commented Jan 23, 2022

I have the impression that this change is no longer needed since Magento 2.4.3 as a similar change was introduced in scope of #31772 apparently.

So this PR can probably be closed if somebody could confirm this? Thanks! 🙂

@theCapypara
Copy link
Member Author

To be honest it is a bit frustrating to contribute to Magento / Adobe Commerce if after 2 years this is the answer and there is still no word from the maintainers. I get that this is just a simple fix and I'm happy to see it fixed somewhere else, but it's still pretty discouraging that this keeps happening. I would love to some communication and quicker processing of Pull Requests in the future or I'll just re-consider whether or not it's worth to even contribute in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Widget Priority: P3 May be fixed according to the position in the backlog. Release Line: 2.4 Risk: low Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Issue] Allow filter directive processors to be used with the Widget template filter
7 participants