Skip to content

Use filters on nested properties#4882

Open
jazayam wants to merge 1 commit into
EasyCorp:4.xfrom
jazayam:feature/nested-filter
Open

Use filters on nested properties#4882
jazayam wants to merge 1 commit into
EasyCorp:4.xfrom
jazayam:feature/nested-filter

Conversation

@jazayam

@jazayam jazayam commented Dec 9, 2021

Copy link
Copy Markdown

This PR makes filters support properties with dot notation by using a new filter as wrapper.

  • The NestedConfigurator is able to configure the wrapped filter by (re)invoking the configurators collection and copy configured FormType and options to display the correct wrapped filter view.

  • The NestedFilter is able to apply required left joins for a given path and delegate query customization to wrapped filter by recreating adapted arguments (like FilterDataDto and EntityDto)

There is no tests for now, but i can add it if you are interested by the purpose.

@jazayam

jazayam commented Dec 9, 2021

Copy link
Copy Markdown
Author

Closes #4881

@javiereguiluz javiereguiluz added this to the 4.x milestone Dec 17, 2021
@npotier

npotier commented Dec 21, 2021

Copy link
Copy Markdown

Thanks @Marcachips for this PR 👍

@DylanKas

Copy link
Copy Markdown

This PR is really interesting and can be very useful. Is there any plan to merge it ?
Does it need some adjustment or help ?

@Ang3

Ang3 commented Apr 20, 2022

Copy link
Copy Markdown
Contributor

Hi,

Any news about this feature?.. We still cannot use filters on embedded properties...
Please review priorities, I think this is a primary feature, more than light/dark mode or others front enhancements...
Many PR's about it for months and nothing. :(

@quentint

Copy link
Copy Markdown
Contributor

Hey there, currently facing an issue this PR would fix, merging would be really great.

Is there anything I/we can do to help?

@25matt12

Copy link
Copy Markdown

Is there any reason this PR is still not merged ? It would be great to have the possibility to filter on nested properties !

@kiler129

kiler129 commented Jan 6, 2023

Copy link
Copy Markdown
Contributor

I think the most important thing here are missing tests - I doubt @javiereguiluz will merge it without them.

@tonyellow

Copy link
Copy Markdown

same here, would be nice if it will be merged.

@msphn

msphn commented Jan 17, 2023

Copy link
Copy Markdown

@Marcachips will you add tests or do you need help?

@jazayam

jazayam commented Jan 18, 2023

Copy link
Copy Markdown
Author

I plan to write tests before the end of this week. I don't need help for now, but thanks 👍

@tonyellow

Copy link
Copy Markdown

@kiler129 i know but this one doesnt work wost nested entities, like product.category

@jazayam jazayam force-pushed the feature/nested-filter branch 2 times, most recently from 79c1acd to cec6ecc Compare January 23, 2023 08:45
@jazayam jazayam force-pushed the feature/nested-filter branch from cec6ecc to af0bb9f Compare January 23, 2023 08:50
@jazayam

jazayam commented Jan 23, 2023

Copy link
Copy Markdown
Author

Tests are pushed !

@Snowbaha

Copy link
Copy Markdown
Contributor

Hello, your PR looks very nice, what are the "blocked point" now?

@mathis-ait

Copy link
Copy Markdown

Hello, this PR will be very helpful

/** @var FilterInterface */
private $wrappedFilter;

public static function new(string $propertyName, string $label = null): self

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't see a reason for this method to be present, it just confuses more. There is no new() method in FilterInterface, so I think this should be removed.

Also, I see a point in renaming wrap() to new() to make it more natural alongside with other filters.

/**
* @author Brandon Marcachi <brandon.marcachi@gmail.com>
*/
final class NestedFilter implements FilterInterface

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you please elaborate a bit: can this be implemented without adding NestedFilter? Why you chose this way?

For me, it looks much more natural to not have a wrapper for filters – similar to Fields. Just add a dot to filter and you're done... Isn't it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hello,

Thank you for the PR and the review. Not sure but I think it's much more complex than just add a dot tbh. Otherwise I really don't understand why this feature is not already implemented. I did a PR for that job and I had to modify much more files to handle all cases due to internal mechanism : #4840

@astronati

Copy link
Copy Markdown

Any news about this PR?

@fdiedler

fdiedler commented Aug 6, 2023

Copy link
Copy Markdown

@Marcachips @javiereguiluz Any news for this great feature ?
I need this feature in my current project :)
Thanks

@jazayam

jazayam commented Aug 16, 2023

Copy link
Copy Markdown
Author

Hello @javiereguiluz !
Can we get visibility into the progress of this Pull Request ?
Do I need to take any additional actions ?

@wehostadm

Copy link
Copy Markdown

Ping of this great feature @javiereguiluz @Marcachips
Hope it will be merged soon :)

@Quentin-Sch

Copy link
Copy Markdown

Ping ?

@Sa-Lat

Sa-Lat commented Oct 18, 2023

Copy link
Copy Markdown

i would also like to have it fixed to get my filter working as intended ♥

@MrJuliuss

Copy link
Copy Markdown
Contributor

Hi, up ?

@BrunoJunior

Copy link
Copy Markdown

@javiereguiluz Some news about this PR ? Thanks

@d3pendent

Copy link
Copy Markdown

I would also welcome this feature.

@JesusRedGar

Copy link
Copy Markdown

No news on this one?

@teklakct

teklakct commented Jun 9, 2024

Copy link
Copy Markdown

If you cannot wait for this MR please check my naive implementation.
You only need to add 2 files to your source code.

https://gist.github.com/teklakct/ed154ccadc18b1463a139f09d3286355

Works with v4.9.4

PS. I think there is an option to slightly enhance the EasyCorp\Bundle\EasyAdminBundle\Factory\FilterFactory and achieve similar behavior. I will try to check this someday.

@ToshY

ToshY commented Dec 31, 2024

Copy link
Copy Markdown

Just got notifications that related issues have been automatically closed (#4881 (comment), #4966 (comment)), so I'm not sure if this will get merged anytime soon 🙁.

@javiereguiluz

Copy link
Copy Markdown
Collaborator

@ToshY we're closing many old issues automatically ... for PRs, I'll review them manually to see if they are still relevant and if we can reuse parts of them.

@ToshY

ToshY commented Dec 31, 2024

Copy link
Copy Markdown

@ToshY we're closing many old issues automatically ... for PRs, I'll review them manually to see if they are still relevant and if we can reuse parts of them.

@javiereguiluz Thanks for the clarification. It would still be nice to have a PR like this getting merged.

@johndodev

johndodev commented Jan 23, 2025

Copy link
Copy Markdown
Contributor

up, I think it's a must have.
Don't know about the PR but The @teklakct solution is woking well and seems relatively simple :
#4882 (comment)

@finnef

finnef commented Feb 3, 2025

Copy link
Copy Markdown
Contributor

I did not have time to do things properly in a fork and PR, but here is the rerolled patch to apply to EA 4.24 to enable the NestedFilter.

ea-filters-nested-properties.patch

$wrappedFilterDto->setProperty($targetProperty);

// Apply required left joins and get the alias we have to work with
$alias = $this->applyLeftJoins($queryBuilder, $filterDataDto->getEntityAlias(), $propertyPath);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This adds support for embeddables.

Suggested change
$alias = $this->applyLeftJoins($queryBuilder, $filterDataDto->getEntityAlias(), $propertyPath);
if ($entityDto->getFqcn() === $targetClassMetadata->getName()) {
$alias = $filterDataDto->getEntityAlias();
} else {
// Apply required left joins and get the alias we have to work with
$alias = $this->applyLeftJoins($queryBuilder, $filterDataDto->getEntityAlias(), $propertyPath);
}

Comment on lines +84 to +94

foreach ($segments as $i => $prop) {
if (!$metadata->hasField($prop) && !$metadata->hasAssociation($prop)) {
self::throwInvalidPropertyPathException($propertyPath, $class);
}

// The target property must be at the end of path
if ($i === $lastIndex) {
$property = $prop;
break;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This adds support for embeddables.

Suggested change
foreach ($segments as $i => $prop) {
if (!$metadata->hasField($prop) && !$metadata->hasAssociation($prop)) {
self::throwInvalidPropertyPathException($propertyPath, $class);
}
// The target property must be at the end of path
if ($i === $lastIndex) {
$property = $prop;
break;
}
$prefix = '';
foreach ($segments as $i => $prop) {
$prop = $prefix.$prop;
if (!$metadata->hasField($prop) && !$metadata->hasAssociation($prop)) {
self::throwInvalidPropertyPathException($propertyPath, $class);
}
// The target property must be at the end of path
if ($i === $lastIndex) {
$property = $prop;
break;
}
if ($metadata->hasField($prop)) {
$prefix .= $prop.'.';
continue;
}

@Crease29

Copy link
Copy Markdown

I think it would be so great if this could get merged.

@Seb33300

Copy link
Copy Markdown
Contributor

Duplicate of #7500

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.