Skip to content

Added test for filtered eager joins#6037

Closed
nicoschoenmaker wants to merge 1 commit intodoctrine:old-prototype-3.xfrom
nicoschoenmaker:bug/filtered-eager-join
Closed

Added test for filtered eager joins#6037
nicoschoenmaker wants to merge 1 commit intodoctrine:old-prototype-3.xfrom
nicoschoenmaker:bug/filtered-eager-join

Conversation

@nicoschoenmaker
Copy link
Copy Markdown

An SQLFilter is not called on collections with eager loading enabled. This PR contains a unit-test that shows the problem.

If I have

  • an order
  • with a product
  • and the product has a list of features
  • where the feature collection has fetch="EAGER" enabled
  • and only visible feature should be shown (enforced by an enabled SQLFilter).

then invisible features are still retrieved from the database!

This PR only contains a unit-test that shows the problem. The test passes if fetch="EAGER" is removed.

Copy link
Copy Markdown
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

Thanks for the work @nicoschoenmaker, could you please apply these changes?

parent::setUp();
try {
$this->_schemaTool->createSchema(array(
$this->_em->getClassMetadata('Doctrine\Tests\ORM\Functional\Order'),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use the ::class syntax instead

$this->_em->flush();

$conf = $this->_em->getConfiguration();
$conf->addFilter('visibility', '\Doctrine\Tests\ORM\Functional\VisibilityFilter');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use the ::class syntax instead

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@nicoschoenmaker you forgot this one 😄

$order = $this->_em->find(Order::class, $order->getId());
$count = count($order->getProduct()->getFeatures());
self::assertContains(
'Doctrine\Tests\ORM\Functional\Feature',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use the ::class syntax instead

public function addFilterConstraint(ClassMetadata $targetEntity, $targetTableAlias)
{
self::$classes[] = $targetEntity->name;
if ($targetEntity->name !== 'Doctrine\Tests\ORM\Functional\Feature') {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use the ::class syntax instead

VisibilityFilter::$classes,
'The VisibilityFilter should have been called for Feature.'
);
self::assertSame(2, $count);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use self::assertCount() instead

@nicoschoenmaker nicoschoenmaker force-pushed the bug/filtered-eager-join branch from 462b4fa to 8eb9fa4 Compare January 16, 2017 07:40
@nicoschoenmaker nicoschoenmaker force-pushed the bug/filtered-eager-join branch from 8eb9fa4 to 4461a6e Compare January 16, 2017 07:47
@nicoschoenmaker
Copy link
Copy Markdown
Author

Thanks for the feedback @lcobucci.

Updated


$order = $this->_em->find(Order::class, $order->getId());
self::assertContains(
'Doctrine\Tests\ORM\Functional\Feature',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And this too

private $visible;

/**
* @todo If we can't replicate it, this used to be Id
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this todo relevant?

public function __construct($description, $visible = true)
{
$this->description = $description;
$this->visible = (bool)$visible;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add space after closing parenthesis

@yp28 yp28 mentioned this pull request Mar 15, 2017
Base automatically changed from master to old-prototype-3.x February 23, 2021 08:18
@github-actions
Copy link
Copy Markdown
Contributor

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

@github-actions github-actions bot added the Stale label Jan 21, 2025
@github-actions
Copy link
Copy Markdown
Contributor

This pull request was closed due to inactivity.

@github-actions github-actions bot closed this Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants