updated addIdentifiersToQuery to work with custom types. #938

oleg-andreyev · 2019-10-14T20:14:31Z

Subject

Batch actions do not work with custom types.

I am targeting this branch, because bug.

Changelog

### Fixed
- `batchAction` now works with custom-types.

Update the tests

core23 · 2019-11-09T19:38:11Z

Can you please add some tests @oleg-andreyev ?

oleg-andreyev · 2019-11-12T11:52:42Z

@core23 planning to find some time on weekends.

stale · 2020-01-30T19:23:46Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

core23 · 2020-01-31T07:59:42Z

Can you please add a test

oleg-andreyev · 2020-01-31T08:46:32Z

@core23 I’ve got message for stale bot and planning to do it on weekdays. Thanks!

src/Model/ModelManager.php

SonataCI · 2020-03-13T14:00:59Z

Could you please rebase your PR and fix merge conflicts?

src/Model/ModelManager.php

VincentLanglet · 2020-03-28T00:10:53Z

Hi @oleg-andreyev ! Do you have time to finish this PR ? This seems like almost done ! :)

oleg-andreyev · 2020-04-09T13:46:57Z

Failing tests aren't related to current PR, can someone please trigger travis manually?

VincentLanglet · 2020-04-09T15:18:16Z

@oleg-andreyev Done

oleg-andreyev · 2020-04-09T15:36:04Z

@core23 please review PR

src/Model/ModelManager.php

VincentLanglet · 2020-04-27T07:44:58Z

@core23 @phansys Can you take a new look at the PR ? :)

src/Model/ModelManager.php

tests/Model/ModelManagerTest.php

SonataCI · 2020-07-26T08:14:37Z

Could you please rebase your PR and fix merge conflicts?

jordisala1991 · 2020-08-24T11:12:11Z

What is the status of this Pr?

VincentLanglet · 2020-11-14T20:01:26Z

Are you still interested by this PR @oleg-andreyev ?

oleg-andreyev · 2020-11-15T15:02:10Z

I'll rebase this branch, but it's taking too long to merge, so I completely forgot about this PR.

VincentLanglet · 2020-11-15T15:17:37Z

I'll rebase this branch, but it's taking too long to merge, so I completely forgot about this PR.

There is few contirbutors, but lot of work/issues/PR.
Don't hesitate to ping us to friendly remember us to take a review.

oleg-andreyev · 2020-11-18T20:15:10Z

tests/Model/ModelManagerTest.php

@@ -982,22 +982,21 @@ public function testGetUrlsafeIdentifierNull(): void
     */
    public function testAddIdentifiersToQuery(array $expectedParameters, array $identifierFieldNames, array $ids): void
    {
-        $modelManager = $this->getMockBuilder(ModelManager::class)


mocking target is a bad practice.

oleg-andreyev · 2020-11-18T20:21:04Z

@OskarStark @core23 @phansys I've rebased branch on latest 3.x, please review.

VincentLanglet · 2020-11-18T20:51:16Z

src/Model/ModelManager.php


-        $prefix = uniqid();
+        $prefix = $this->generateIdentifierPrefix();


Why introducing this method ? (Especially when we're trying to avoid protected method).

to have an ability to override it in tests, otherwise, it's impossible to predict results with hardcoded unique

We shouldn't add method for this.
You could check the name by using a regex instead.

One good practice from DDD - avoid having dependencies on I/O outside of the Infrastructure layer, so I tend to remove such dependencies into a separate method/class so that it could be replaced later without affecting main functionality and tests. What do you think about it?

I like the idea 💡

@oleg-andreyev In Sonata 4.0, the ModelManager will be final.

So a protected method is useless, it's basically a private one.

And creating a private method for calling uniqid() seems useless to me.

VincentLanglet · 2020-11-18T21:03:30Z

src/Model/ModelManager.php

+        return Type::getType($fieldType);
+    }
+
+    private function buildInnerIdentifier(string $prefix, string $field, $value, int $pos, QueryBuilder $qb, ClassMetadata $classMetadata): string


There is a lot of parameters in this private method.

Is there a way to reduce the number ?
Maybe the parameterName can be an argument instead of passing the prefix and the pos.

Initially wanted to make addIdentifiersToQuery a bit cleaner and remove chunk of code from foreach-loop but other hand I can revert this private method

The method addIdentifiersToQuery is complex, so this private method has a lot of sens.

$parameterName = sprintf('field_%s_%s_%d', $prefix, $name, $pos); $this-> buildInnerIdentifier($field, $value, $paraterName, $qb, $classMetadata)

Was just a suggestion, not a blocking point.

It will reduce a few args. Good idea. Will do it.

VincentLanglet · 2020-11-18T21:04:44Z

src/Model/ModelManager.php

+            if ($this->hasFieldCustomType($fieldType)) {
+                $type = $this->getFieldCustomType($fieldType);


When I look at the logic inside these function, I'm not sure we need to add private method for this.

It's used in two places. I usually try to remove duplicates. This reduces the burden on the developers when they need to update "Type" class usage.

Yes, I saw the duplicate.

But it's basically like a method

private function foo($a) { return $this->bar($a); }

Right. Similar to findBy and findOneBy

SonataDoctrineORMAdminBundle/src/Model/ModelManager.php

Lines 285 to 293 in 560e9f0

public function findBy($class, array $criteria = [])

{

return $this->getEntityManager($class)->getRepository($class)->findBy($criteria);

}

public function findOneBy($class, array $criteria = [])

{

return $this->getEntityManager($class)->getRepository($class)->findOneBy($criteria);

}

But these one are public, and required by the interface.

src/Model/ModelManager.php

- updated addIdentifiersToQuery to work with custom types. - added test for addIdentifiersToQuery: testAddIdentifiersToQueryWithCustomIdMapping - extracted Type operations into getFieldCustomType, extracted loop-part from addIdentifiersToQuery to buildInnerIdentifier

oleg-andreyev · 2020-12-06T23:23:06Z

@core23 @OskarStark @phansys can you please take a look?

VincentLanglet · 2020-12-14T15:38:31Z

src/Model/ModelManager.php


-        $prefix = uniqid();
+        $prefix = $this->generateIdentifierPrefix();


@oleg-andreyev In Sonata 4.0, the ModelManager will be final.

So a protected method is useless, it's basically a private one.

And creating a private method for calling uniqid() seems useless to me.

github-actions · 2021-06-12T16:11:57Z

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

greg0ire added the patch label Oct 14, 2019

core23 added Needs: Tests pending author labels Dec 23, 2019

stale bot added the wontfix label Jan 30, 2020

SonataCI removed the wontfix label Jan 30, 2020

SonataCI removed the pending author label Jan 31, 2020

oleg-andreyev force-pushed the batch-actions-does-not-work-with-custom-type branch from b478c30 to cc77f69 Compare February 2, 2020 20:01

core23 requested changes Feb 2, 2020

View reviewed changes

src/Model/ModelManager.php Outdated Show resolved Hide resolved

OskarStark previously approved these changes Feb 2, 2020

View reviewed changes

oleg-andreyev mentioned this pull request Feb 2, 2020

Added check for empty SELECT expression doctrine/orm#8010

Open

oleg-andreyev requested a review from core23 February 5, 2020 13:56

core23 requested changes Mar 13, 2020

View reviewed changes

src/Model/ModelManager.php Outdated Show resolved Hide resolved

oleg-andreyev dismissed OskarStark’s stale review via 116056c March 29, 2020 20:37

oleg-andreyev force-pushed the batch-actions-does-not-work-with-custom-type branch from d2d6792 to 116056c Compare March 29, 2020 20:37

VincentLanglet closed this Apr 9, 2020

VincentLanglet reopened this Apr 9, 2020

phansys requested changes Apr 9, 2020

View reviewed changes

src/Model/ModelManager.php Outdated Show resolved Hide resolved

OskarStark removed the Needs: Tests label Apr 19, 2020

phansys requested changes Apr 27, 2020

View reviewed changes

oleg-andreyev requested review from phansys and core23 May 28, 2020 17:02

phansys previously approved these changes May 28, 2020

View reviewed changes

phansys added the pending author label Aug 29, 2020

SonataCI removed the pending author label Nov 15, 2020

core23 previously approved these changes Nov 15, 2020

View reviewed changes

oleg-andreyev dismissed stale reviews from core23 and phansys via 6553c7d November 18, 2020 20:14

oleg-andreyev force-pushed the batch-actions-does-not-work-with-custom-type branch from 63c36cd to 6553c7d Compare November 18, 2020 20:14

oleg-andreyev commented Nov 18, 2020

View reviewed changes

oleg-andreyev requested review from OskarStark, phansys and core23 November 18, 2020 20:20

VincentLanglet reviewed Nov 18, 2020

View reviewed changes

oleg-andreyev and others added 2 commits November 18, 2020 23:54

!squash extracted $parameterName outside of buildInnerIdentifier

16efac2

oleg-andreyev force-pushed the batch-actions-does-not-work-with-custom-type branch from 560e9f0 to 16efac2 Compare November 18, 2020 21:54

VincentLanglet requested a review from a team December 14, 2020 15:33

VincentLanglet requested changes Dec 14, 2020

View reviewed changes

VincentLanglet requested a review from a team December 14, 2020 15:39

github-actions bot added the stale label Jun 12, 2021

github-actions bot closed this Jun 19, 2021


		$prefix = uniqid();
		$prefix = $this->generateIdentifierPrefix();

		if ($this->hasFieldCustomType($fieldType)) {
		$type = $this->getFieldCustomType($fieldType);

	public function findBy($class, array $criteria = [])
	{
	return $this->getEntityManager($class)->getRepository($class)->findBy($criteria);
	}

	public function findOneBy($class, array $criteria = [])
	{
	return $this->getEntityManager($class)->getRepository($class)->findOneBy($criteria);
	}

updated addIdentifiersToQuery to work with custom types. #938

updated addIdentifiersToQuery to work with custom types. #938

Conversation

oleg-andreyev commented Oct 14, 2019 • edited

Subject

Changelog

core23 commented Nov 9, 2019

oleg-andreyev commented Nov 12, 2019

stale bot commented Jan 30, 2020

core23 commented Jan 31, 2020

oleg-andreyev commented Jan 31, 2020

SonataCI commented Mar 13, 2020

VincentLanglet commented Mar 28, 2020

oleg-andreyev commented Apr 9, 2020

VincentLanglet commented Apr 9, 2020

oleg-andreyev commented Apr 9, 2020

VincentLanglet commented Apr 27, 2020

SonataCI commented Jul 26, 2020

jordisala1991 commented Aug 24, 2020

VincentLanglet commented Nov 14, 2020

oleg-andreyev commented Nov 15, 2020 • edited

VincentLanglet commented Nov 15, 2020

Choose a reason for hiding this comment

oleg-andreyev commented Nov 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-andreyev commented Dec 6, 2020

Choose a reason for hiding this comment

github-actions bot commented Jun 12, 2021

oleg-andreyev commented Oct 14, 2019 •

edited

oleg-andreyev commented Nov 15, 2020 •

edited