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

Make ancestorClass option consistency to underscore #351

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

shakaran
Copy link
Contributor

Applied renaming over the repo with:

grep -ril ancestorClass | xargs sed -i 's@ancestorClass@ancestor_class@g'

Copy link
Collaborator

@garak garak left a comment

Choose a reason for hiding this comment

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

We can't just change an option like this.
We need to keep BC: add new option, deprecate old option. Then we can remove old option in the next major version.

@shakaran
Copy link
Contributor Author

We can't just change an option like this.
We need to keep BC: add new option, deprecate old option. Then we can remove old option in the next major version.

I just add deprecation contracts as symfony way and allow the support for both options during the transition for avoid BC. Let me know if it suitable for you now

@shakaran shakaran requested a review from garak August 31, 2021 12:34
src/Knp/Menu/Renderer/ListRenderer.php Outdated Show resolved Hide resolved
doc/examples/01_apply_active_class_to_whole_tree.md Outdated Show resolved Hide resolved
@shakaran shakaran requested a review from garak August 31, 2021 12:47
src/Knp/Menu/Renderer/ListRenderer.php Outdated Show resolved Hide resolved
@@ -55,7 +55,8 @@
{%- if matcher.isCurrent(item) %}
{%- set classes = classes|merge([options.currentClass]) %}
{%- elseif matcher.isAncestor(item, options.matchingDepth) %}
{%- set classes = classes|merge([options.ancestorClass]) %}
{# ancestorClass will be deprecated in future, use ancestor_class #}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this does not report the deprecation AFAICT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 24fa9c2

composer.json Outdated Show resolved Hide resolved
@@ -62,7 +62,7 @@ jobs:
with:
php-version: ${{ matrix.php }}
- run: |
sed -ri 's/"symfony\/(.+)": "(.+)"/"symfony\/\1": "'${{ matrix.symfony }}'"/' composer.json;
sed -ri '/symfony\/deprecation-contracts/!s/"symfony\/(.+)\symfony\/deprecation-contracts/;": "(.+)"/"symfony\/\1": "'${{ matrix.symfony }}'"/' composer.json;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need a space between ! and s

Copy link
Collaborator

Choose a reason for hiding this comment

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

you added it before the wrong s...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, this was giving me a headache. Thanks for the charm.

image

Ready to merge?

image

@shakaran shakaran requested review from garak and stof September 1, 2021 10:23
@shakaran
Copy link
Contributor Author

shakaran commented Sep 2, 2021

@stof as soon you can give me your RTM and merge it I will sent the next PR for the other options described in #171

@shakaran
Copy link
Contributor Author

Please, let me know if there are something more needed to get this merged. I would like sent the next PR with the other options

@shakaran
Copy link
Contributor Author

One month later, I still didn't receive feedback about why the PR is not merged or if I need to change something.

@garak
Copy link
Collaborator

garak commented Oct 13, 2021

One month later, I still didn't receive feedback about why the PR is not merged or if I need to change something.

I'm sorry to see your expectations about time disappointed. Unfortunately, there's no guarantee about that, since everyone is working in their free time.

@shakaran
Copy link
Contributor Author

Please @stof there are any chance that this will be approved soon? I would like integrate the changes in other of my projects

It should be quick to check for you. Thanks for your time.

@shakaran
Copy link
Contributor Author

Should I fork this project and create a parallel project since maintainers doesn't have time to attend issues or resolve the pull request?

I know that everybody is working in his free time here.

But if main maintainers don't have time, the open source community dies, and the workaround is assign new people that wants to contribute, or in my side I only have the option to fork (which I don't like to do it, but if more time without response or progress I will have to initiate)

@stof
Copy link
Collaborator

stof commented Jan 26, 2022

The issue I have with accepting this PR is that I'm totally unsure about the effectiveness of the BC layer. There are 2 places impacted by this renaming:

  • passing the option to knp_menu_render: this BC layer is easy
  • using the option in the template: this one is hard, because we need to account for projects using custom templates (either migrated or not migrated yet), and for custom templates that then extend our default template

And I'm even less sure about the effectiveness of the deprecation warnings for that second case.

This means that this PR is risky, without much benefit (the existing name still works fine), while I only have limited free time dedicated to this project, and the same seems to be true for @garak

@shakaran
Copy link
Contributor Author

First, thanks @stof for your time and quick response today.

The issue I have with accepting this PR is that I'm totally unsure about the effectiveness of the BC layer. There are 2 places impacted by this renaming:

  • passing the option to knp_menu_render: this BC layer is easy

I try to catch the deprecation in twig render here 4abbba4

The test is not fully working, I don't know exactly how to mock the deprecation, so I need a bit of help with that lines. Peer review and suggestions are appreciated.

  • using the option in the template: this one is hard, because we need to account for projects using custom templates (either migrated or not migrated yet), and for custom templates that then extend our default template

I am not sure of this case, could you provide a example of project in that situation, so I can see the problem?

And I'm even less sure about the effectiveness of the deprecation warnings for that second case.

In theory, if this is released under a new version, they can use old version without BC until we can figure out how to address the problem.

This means that this PR is risky, without much benefit (the existing name still works fine), while I only have limited free time dedicated to this project, and the same seems to be true for @garak

The idea here when this PR gets accepted, is send more PR with the other names affected and close the issue of options consistency #171

@stof
Copy link
Collaborator

stof commented Jan 26, 2022

@shakaran the whole point of using deprecations is to allow to release the change in a minor version (so without BC breaks) and having projects than migrate their code based on the warnings they get.
Just doing a major version to allow us to do BC breaks without a BC layer is not a good option: projects won't know what they need to change to be compatible, which is a bad upgrade path. And this would not even fail when they fail to migrate, but more likely silently ignore their options, which is the worse kind of BC breaks as you might never discover that you are affected, until you do some WTF debugging long after upgrading the package.

The idea here when this PR gets accepted, is send more PR with the other names affected and close the issue of options consistency #171

and fixing this consistency issue is about theoretical purity, not about actually making this package better for its user. The benefit of fixing this is small (it would help people learn the right option name without looking in the doc), but doing so at a high cost would be a no-go.
My comment from 2014 on this issue still holds: fixing this inconsistency would be good if we can provide a full BC layer. Otherwise, the cost on the ecosystem is too high (and the fact that I have limited time to spend on that project means that I definitely don't want to create a situation where upgrade paths provide WTF situations where users would then report issues on the repo to get help that I would not have time to provide)

The test is not fully working, I don't know exactly how to mock the deprecation, so I need a bit of help with that lines.

When using the symfony/phpunit-bridge deprecation reporting for the testsuite (which we do), you need to use the trait of that package to define deprecation expectations instead of relying on the expectDeprecation feature provided by PHPUnit itself.

@shakaran
Copy link
Contributor Author

@shakaran the whole point of using deprecations is to allow to release the change in a minor version (so without BC breaks) and having projects than migrate their code based on the warnings they get. Just doing a major version to allow us to do BC breaks without a BC layer is not a good option: projects won't know what they need to change to be compatible, which is a bad upgrade path. And this would not even fail when they fail to migrate, but more likely silently ignore their options, which is the worse kind of BC breaks as you might never discover that you are affected, until you do some WTF debugging long after upgrading the package.

Ok, minor release then (3.3) as pointed. As far it gets merged and released finally.

The idea here when this PR gets accepted, is send more PR with the other names affected and close the issue of options consistency #171

and fixing this consistency issue is about theoretical purity, not about actually making this package better for its user. The benefit of fixing this is small (it would help people learn the right option name without looking in the doc), but doing so at a high cost would be a no-go. My comment from 2014 on this issue still holds: fixing this inconsistency would be good if we can provide a full BC layer. Otherwise, the cost on the ecosystem is too high (and the fact that I have limited time to spend on that project means that I definitely don't want to create a situation where upgrade paths provide WTF situations where users would then report issues on the repo to get help that I would not have time to provide)

After this PR gets accepted I will provide the other cases and options rewriten with BC layer support of deprecations.

The test is not fully working, I don't know exactly how to mock the deprecation, so I need a bit of help with that lines.

When using the symfony/phpunit-bridge deprecation reporting for the testsuite (which we do), you need to use the trait of that package to define deprecation expectations instead of relying on the expectDeprecation feature provided by PHPUnit itself.

ok, I added, but it seems that for Symfony 4.4 with PHP 7.4 it breaks since no trait available

@stof
Copy link
Collaborator

stof commented Jan 26, 2022

@shakaran this is an issue in the way Symfony-specific jobs are configured. The phpunit-bridge should not be affected (it is a special component, that is not replaced by the monorepo package either)

@shakaran
Copy link
Contributor Author

e in the way Symfo

ok, then, it is good to merge, or do you need that I make more changes? Thanks

@shakaran
Copy link
Contributor Author

@stof almost a year for get this merged. Please, could you consider it? Thanks, only remains your approve vote

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.

3 participants