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
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
ff8864b
Make ancestorClass option consistency to underscore
shakaran Aug 31, 2021
4fc676d
Require symfony/deprecation-contracts for trigger warnings
shakaran Aug 31, 2021
df85637
Add deprecation contracts and allow both ways for support BC
shakaran Aug 31, 2021
d2b325b
Fix if spaced as per review suggested
shakaran Aug 31, 2021
d7a3399
Revert changes in doc for deprecated way as per review suggested
shakaran Aug 31, 2021
3aeb687
Update version number for deprecation
shakaran Aug 31, 2021
24fa9c2
Improve warning message for twig deprecation as per review suggested
shakaran Aug 31, 2021
4103108
Merge branch 'master' into ancestor_class_underscore
shakaran Aug 31, 2021
05e3231
Update test for show as legacy deprecation warnings
shakaran Aug 31, 2021
5bf59b9
Avoid duplication of configs and show deprecations warnings too
shakaran Aug 31, 2021
5ae265b
Update php version in composer
shakaran Aug 31, 2021
10569aa
Try to avoid replacements by deprecation contracts in github build
shakaran Aug 31, 2021
abdeda8
Try another sed vudu way
shakaran Aug 31, 2021
ae50e57
Add exclusion pattern /pattern/!s/pattern/
shakaran Aug 31, 2021
98046aa
Update the sed vudu charm
shakaran Aug 31, 2021
4ef0026
Vudu sed require a space
shakaran Aug 31, 2021
d82c70f
Update build.yaml
garak Aug 31, 2021
4abbba4
Add support deprecation for knp_menu_render and knp_menu_get with anc…
shakaran Jan 26, 2022
9c5797c
Use ExpectDeprecationTrait for deprecation message
shakaran Jan 26, 2022
78608e3
Add @group Legacy or Legacy in name for allow pass test in CI
shakaran Jan 26, 2022
742aacc
Mistake preprend at beginning legacy
shakaran Jan 26, 2022
9de5229
Add as annotation since CI don't recognize
shakaran Jan 26, 2022
c42343b
Remove message since expects empty message for trait
shakaran Jan 26, 2022
a4bc394
Add empty string message for deprecation instead null
shakaran Jan 26, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/build.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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

if: matrix.symfony
- run: composer config minimum-stability dev && composer config prefer-stable true
if: matrix.dev
Expand Down
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
}
],
"require": {
"php": "^7.3 || ^8.0"
"php": "^7.3 || ^8.0",
"symfony/deprecation-contracts": "^2.4"
},
"conflict": {
"twig/twig": "<1.40 || >=2,<2.9"
Expand Down
2 changes: 1 addition & 1 deletion doc/01-Basic-Menus.md
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ the second argument to the `render()` method:
is an ancestor of the current item.
* `currentAsLink` (default: `true`): Whether to render the "current" menu item as link or as span.
* `currentClass` (default: `current`)
* `ancestorClass` (default: `current_ancestor`)
* `ancestor_class` (default: `current_ancestor`) (alias: ancestorClass, currently deprecated)
* `firstClass` (default: `first`)
* `lastClass` (default: `last`)
* `compressed` (default: `false`)
Expand Down
4 changes: 2 additions & 2 deletions doc/examples/01_apply_active_class_to_whole_tree.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ render method of your used renderer:

```php
<?php
$renderer->render($item, ['currentClass' => 'active', 'ancestorClass' => 'active']);
$renderer->render($item, ['currentClass' => 'active', 'ancestor_class' => 'active']);
```

or pass it as argument to the constructor of the used renderer:

```php
<?php
$renderer = new Renderer($matcher, ['currentClass' => 'active', 'ancestorClass' => 'active']);
$renderer = new Renderer($matcher, ['currentClass' => 'active', 'ancestor_class' => 'active']);
```
15 changes: 13 additions & 2 deletions src/Knp/Menu/Renderer/ListRenderer.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public function __construct(MatcherInterface $matcher, array $defaultOptions = [
'matchingDepth' => null,
'currentAsLink' => true,
'currentClass' => 'current',
'ancestorClass' => 'current_ancestor',
'ancestor_class' => 'current_ancestor',
'firstClass' => 'first',
'lastClass' => 'last',
'compressed' => false,
Expand All @@ -48,6 +48,13 @@ public function render(ItemInterface $item, array $options = []): string
{
$options = \array_merge($this->defaultOptions, $options);

// Avoid duplication of current_ancestor class. Overwrite value in old config to new one
if (isset($options['ancestorClass'])) {
$options['ancestor_class'] = $options['ancestorClass'];
unset($options['ancestorClass']);
trigger_deprecation('knplabs/knp-menu', '3.3', 'Using "%s" option is deprecated, use "%s" instead.', 'ancestorClass', 'ancestor_class');
}

$html = $this->renderList($item, $item->getChildrenAttributes(), $options);

if ($options['clear_matcher']) {
Expand Down Expand Up @@ -130,7 +137,11 @@ protected function renderItem(ItemInterface $item, array $options): string
if ($this->matcher->isCurrent($item)) {
$class[] = $options['currentClass'];
} elseif ($this->matcher->isAncestor($item, $options['matchingDepth'])) {
$class[] = $options['ancestorClass'];
if (isset($options['ancestorClass'])) { // Deprecated: it will be removed in future (@see: ancestor_class)
trigger_deprecation('knplabs/knp-menu', '3.3', 'Using "%s" option is deprecated, use "%s" instead.', 'ancestorClass', 'ancestor_class');
$options['ancestor_class'] = $options['ancestorClass'];
}
$class[] = $options['ancestor_class'];
}

if ($item->actsLikeFirst()) {
Expand Down
2 changes: 1 addition & 1 deletion src/Knp/Menu/Renderer/RendererInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ interface RendererInterface
* 1: only direct children
* - currentAsLink: whether the current item should be a link
* - currentClass: class added to the current item
* - ancestorClass: class added to the ancestors of the current item
* - ancestor_class: class added to the ancestors of the current item (alias: ancestorClass, deprecated in a future)
* - firstClass: class added to the first child
* - lastClass: class added to the last child
*
Expand Down
9 changes: 8 additions & 1 deletion src/Knp/Menu/Renderer/TwigRenderer.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public function __construct(
'matchingDepth' => null,
'currentAsLink' => true,
'currentClass' => 'current',
'ancestorClass' => 'current_ancestor',
'ancestor_class' => 'current_ancestor',
'firstClass' => 'first',
'lastClass' => 'last',
'template' => $template,
Expand All @@ -55,6 +55,13 @@ public function render(ItemInterface $item, array $options = []): string
{
$options = \array_merge($this->defaultOptions, $options);

// Avoid duplication of current_ancestor class. Overwrite value in old config to new one
if (isset($options['ancestorClass'])) {
$options['ancestor_class'] = $options['ancestorClass'];
unset($options['ancestorClass']);
trigger_deprecation('knplabs/knp-menu', '3.3', 'Using "%s" option is deprecated, use "%s" instead.', 'ancestorClass', 'ancestor_class');
}

$html = $this->environment->render($options['template'], ['item' => $item, 'options' => $options, 'matcher' => $this->matcher]);

if ($options['clear_matcher']) {
Expand Down
6 changes: 5 additions & 1 deletion src/Knp/Menu/Resources/views/knp_menu.html.twig
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,11 @@
{%- if matcher.isCurrent(item) %}
{%- set classes = classes|merge([options.currentClass]) %}
{%- elseif matcher.isAncestor(item, options.matchingDepth) %}
{%- set classes = classes|merge([options.ancestorClass]) %}
{%- set classes = classes|merge([options.ancestor_class]) %}
{%- if options.ancestorClass is not empty %}
{% deprecated 'knplabs/knp-menu 3.3: Using "ancestorClass" option is deprecated, use "ancestor_class" instead.' %}
{%- set classes = classes|merge([options.ancestorClass]) %}
{%- endif %}
{%- endif %}
{%- if item.actsLikeFirst %}
{%- set classes = classes|merge([options.firstClass]) %}
Expand Down
4 changes: 2 additions & 2 deletions tests/Knp/Menu/Tests/Renderer/AbstractRendererTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ public function testRenderWithClassAndTitle(): void
$this->assertEquals($rendered, $this->renderer->render($this->menu));
}

public function testRenderWithCurrentItem(): void
public function testLegacyRenderWithCurrentItem(): void
{
$this->ch2->setCurrent(true);
$rendered = '<ul class="root"><li class="current_ancestor first"><span>Parent 1</span><ul class="menu_level_1"><li class="first"><span>Child 1</span></li><li class="current"><span>Child 2</span></li><li class="last"><span>Child 3</span></li></ul></li><li class="last"><span>Parent 2</span><ul class="menu_level_1"><li class="first last"><span>Child 4</span><ul class="menu_level_2"><li class="first last"><span>Grandchild 1</span></li></ul></li></ul></li></ul>';
Expand Down Expand Up @@ -326,7 +326,7 @@ public function testMatchingDepth1(): void
$this->assertEquals($rendered, $this->renderer->render($this->menu, ['depth' => 1, 'matchingDepth' => 2]));
}

public function testMatchingDepth2(): void
public function testLegacyMatchingDepth2(): void
{
$this->menu['Parent 1']['Child 1']->setCurrent(true);
$rendered = '<ul class="root"><li class="first"><span>Parent 1</span></li><li class="last"><span>Parent 2</span></li></ul>';
Expand Down