-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Optimize matching zones to a given address #15275
Merged
Merged
Changes from 16 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
84aaf55
Add eager loading for Zone's members
jakubtobiasz ba06fc1
Add a method finding all zones by address to the Zone's repository
jakubtobiasz a69fb7a
Add a method finding all zones by a zone to the Zone's repository
jakubtobiasz 8b6c365
Implement more optimized ZoneMatcher::matchAll method
jakubtobiasz 660c95d
Implement more optimized ZoneMatcher::match method
jakubtobiasz 43004fe
Provide ECS fixes
jakubtobiasz 5c6033d
Move ZoneRepositoryInterface from the Bundle to the Component
jakubtobiasz 7491c61
Take into account 'all' scope in the Zone
jakubtobiasz 5a3923a
Add AliceDataFixturesBundle to the test app in the AddressingBundle
jakubtobiasz 3287b8c
Add selecting zone members to prevent lazy loading them
jakubtobiasz 7bf31b2
Add purging the test database between phpunit tests
jakubtobiasz 830c741
Remove the visibility definitions for methods in the spec
jakubtobiasz c152bb5
Make the createFindByAddressQueryBuilder public
jakubtobiasz 7e975d3
Replace overriding the zones argument with a new zonesCodes variable
jakubtobiasz bea9452
Fix database-based tests
jakubtobiasz ce72d4b
Rename the findAllByZones method to findZonesByMembers
jakubtobiasz 6e5bcee
Remove the unnecessary eager loading of zone's members
jakubtobiasz 68a47ca
Rename the "findZonesByMembers" method to "findByMembers"
jakubtobiasz 66cfe9c
Rename the "findAllByAddress" method to "findByAddress"
jakubtobiasz 8922d7e
Rename the "createFindByAddressQueryBuilder" method to "createByAddre…
jakubtobiasz fc261db
Adjust scopes' names to better reflect Sylius' zone scopes
jakubtobiasz 89a5e8a
Adjust tables' aliases
jakubtobiasz 8365fbc
Replace the `findOneByAddress` method with `findOneByAddressAndType`
jakubtobiasz d72ffda
Rename $query variables to $queryBuilder
jakubtobiasz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
126 changes: 126 additions & 0 deletions
126
src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,126 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Sylius package. | ||
* | ||
* (c) Sylius Sp. z o.o. | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Sylius\Bundle\AddressingBundle\Repository; | ||
|
||
use Doctrine\ORM\QueryBuilder; | ||
use Sylius\Bundle\ResourceBundle\Doctrine\ORM\EntityRepository; | ||
use Sylius\Component\Addressing\Model\AddressInterface; | ||
use Sylius\Component\Addressing\Model\Scope; | ||
use Sylius\Component\Addressing\Model\ZoneInterface; | ||
use Sylius\Component\Addressing\Repository\ZoneRepositoryInterface; | ||
|
||
/** | ||
* @implements ZoneRepositoryInterface<ZoneInterface> | ||
*/ | ||
class ZoneRepository extends EntityRepository implements ZoneRepositoryInterface | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fact that a new repository for an existing resource has shown up should be noted in the upgrade file. If someone has already created a custom one prior, the matcher will just fail. |
||
{ | ||
public function findOneByAddress(AddressInterface $address, ?string $scope = null): ?ZoneInterface | ||
{ | ||
$query = $this->createFindByAddressQueryBuilder($address, $scope); | ||
|
||
$query | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
->addSelect('(CASE | ||
WHEN z.type = \'province\' THEN 1 | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
WHEN z.type = \'country\' THEN 2 | ||
WHEN z.type = \'zone\' THEN 3 | ||
ELSE 4 | ||
END) AS HIDDEN sort_order') | ||
->orderBy('sort_order', 'ASC') | ||
->setMaxResults(1) | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
; | ||
|
||
return $query->getQuery()->getOneOrNullResult(); | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/** @return ZoneInterface[] */ | ||
public function findAllByAddress(AddressInterface $address, ?string $scope = null): array | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
return $this->createFindByAddressQueryBuilder($address, $scope)->getQuery()->getResult(); | ||
} | ||
|
||
public function createFindByAddressQueryBuilder(AddressInterface $address, ?string $scope = null): QueryBuilder | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
$query = $this->createQueryBuilder('z') | ||
->select('z', 'm') | ||
->leftJoin('z.members', 'm') | ||
; | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (null !== $scope) { | ||
$query | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
->andWhere($query->expr()->in('z.scope', ':scopes')) | ||
->setParameter('scopes', [$scope, Scope::ALL]) | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
; | ||
} | ||
|
||
$orConditions = []; | ||
|
||
if ($address->getCountryCode() !== null) { | ||
$orConditions[] = $query->expr()->andX( | ||
$query->expr()->eq('z.type', ':country'), | ||
$query->expr()->eq('m.code', ':countryCode'), | ||
); | ||
|
||
$query->setParameter('country', ZoneInterface::TYPE_COUNTRY); | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
$query->setParameter('countryCode', $address->getCountryCode()); | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
if ($address->getProvinceCode() !== null) { | ||
$orConditions[] = $query->expr()->andX( | ||
$query->expr()->eq('z.type', ':province'), | ||
$query->expr()->eq('m.code', ':provinceCode'), | ||
); | ||
|
||
$query->setParameter('province', ZoneInterface::TYPE_PROVINCE); | ||
$query->setParameter('provinceCode', $address->getProvinceCode()); | ||
} | ||
|
||
$query->andWhere($query->expr()->orX(...$orConditions)); | ||
|
||
return $query; | ||
} | ||
|
||
/** | ||
* @param array<ZoneInterface> $members | ||
* | ||
* @return array<ZoneInterface> | ||
*/ | ||
public function findZonesByMembers(array $members, ?string $scope = null): array | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
$zonesCodes = array_map( | ||
fn (ZoneInterface $zone): string => $zone->getCode(), | ||
$members, | ||
); | ||
|
||
$query = $this->createQueryBuilder('z') | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
->select('z', 'm') | ||
->leftJoin('z.members', 'm') | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
; | ||
|
||
if (null !== $scope) { | ||
$query | ||
->andWhere($query->expr()->in('z.scope', ':scopes')) | ||
->setParameter('scopes', [$scope, Scope::ALL]) | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
; | ||
} | ||
|
||
$query | ||
->andWhere('z.type = :type') | ||
->andWhere($query->expr()->in('m.code', ':zones')) | ||
->setParameter('type', ZoneInterface::TYPE_ZONE) | ||
->setParameter('zones', $zonesCodes) | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
; | ||
|
||
return $query->getQuery()->getResult(); | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
86 changes: 86 additions & 0 deletions
86
src/Sylius/Bundle/AddressingBundle/Tests/Matcher/ZoneMatcherTest.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Sylius package. | ||
* | ||
* (c) Sylius Sp. z o.o. | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Sylius\Bundle\AddressingBundle\Tests\Matcher; | ||
|
||
use Doctrine\Common\DataFixtures\Purger\ORMPurger; | ||
use Doctrine\ORM\EntityManagerInterface; | ||
use Doctrine\Persistence\ObjectManager; | ||
use Fidry\AliceDataFixtures\LoaderInterface; | ||
use Fidry\AliceDataFixtures\Persistence\PurgeMode; | ||
use Sylius\Component\Addressing\Model\Address; | ||
use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase; | ||
|
||
final class ZoneMatcherTest extends KernelTestCase | ||
{ | ||
protected function setUp(): void | ||
{ | ||
$this->loadFixtures(); | ||
} | ||
|
||
/** @test */ | ||
public function it_matches_all_zones_with_their_parents(): void | ||
{ | ||
$address = new Address(); | ||
$address->setCountryCode('PL'); | ||
$address->setProvinceCode('MA'); | ||
|
||
$zoneMatcher = self::getContainer()->get('sylius.zone_matcher'); | ||
$matchedZones = []; | ||
|
||
foreach ($zoneMatcher->matchAll($address) as $zone) { | ||
$matchedZones[$zone->getCode()] = $zone; | ||
} | ||
|
||
$this->assertCount(4, $matchedZones); | ||
$this->assertArrayHasKey('EU', $matchedZones); | ||
$this->assertArrayHasKey('VISEGRAD_GROUP', $matchedZones); | ||
$this->assertArrayHasKey('PL', $matchedZones); | ||
$this->assertArrayHasKey('NATO', $matchedZones); | ||
} | ||
|
||
/** @test */ | ||
public function it_matches_all_zones_with_their_parents_with_restricting_by_scope(): void | ||
{ | ||
$address = new Address(); | ||
$address->setCountryCode('PL'); | ||
$address->setProvinceCode('MA'); | ||
|
||
$zoneMatcher = self::getContainer()->get('sylius.zone_matcher'); | ||
$matchedZones = []; | ||
|
||
foreach ($zoneMatcher->matchAll($address, 'nato') as $zone) { | ||
$matchedZones[$zone->getCode()] = $zone; | ||
} | ||
|
||
$this->assertCount(3, $matchedZones); | ||
$this->assertArrayHasKey('NATO', $matchedZones); | ||
$this->assertArrayHasKey('PL', $matchedZones); | ||
$this->assertArrayHasKey('VISEGRAD_GROUP', $matchedZones); | ||
} | ||
|
||
private function loadFixtures(): void | ||
{ | ||
/** @var LoaderInterface $fixtureLoader */ | ||
$fixtureLoader = self::getContainer()->get('fidry_alice_data_fixtures.loader.doctrine'); | ||
|
||
/** @var EntityManagerInterface $manager */ | ||
$manager = self::getContainer()->get('doctrine.orm.default_entity_manager'); | ||
|
||
(new ORMPurger($manager))->purge(); | ||
|
||
$fixtureLoader->load([ | ||
__DIR__ . '/ZoneMatcherTest/fixtures.yaml', | ||
], [], [], PurgeMode::createDeleteMode()); | ||
} | ||
} |
42 changes: 42 additions & 0 deletions
42
src/Sylius/Bundle/AddressingBundle/Tests/Matcher/ZoneMatcherTest/fixtures.yaml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
Sylius\Component\Addressing\Model\Country: | ||
country_PL: | ||
code: PL | ||
|
||
Sylius\Component\Addressing\Model\Province: | ||
province_{MA, LD, MZ, DS}: | ||
code: <current()> | ||
name: <current()> | ||
abbreviation: <current()> | ||
country: "@country_PL" | ||
|
||
Sylius\Component\Addressing\Model\ZoneMember: | ||
member_province_{MA, LD, MZ, DS}: | ||
code: <current()> | ||
member_{poland_visegrad_group, poland_nato}: | ||
code: PL | ||
member_visegrad_group: | ||
code: "VISEGRAD_GROUP" | ||
|
||
Sylius\Component\Addressing\Model\Zone: | ||
zone_poland: | ||
code: PL | ||
name: Poland | ||
type: province | ||
members: ["@member_province_MA", "@member_province_LD", "@member_province_MZ", "@member_province_DS"] | ||
zone_visegrad_group: | ||
code: VISEGRAD_GROUP | ||
name: Visegrad Group | ||
type: zone | ||
members: ["@member_poland_visegrad_group"] | ||
zone_eu: | ||
code: EU | ||
name: European Union | ||
type: zone | ||
members: ["@member_visegrad_group"] | ||
scope: 'only_eu' | ||
jakubtobiasz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
zone_nato: | ||
code: NATO | ||
name: NATO | ||
type: zone | ||
members: ["@member_poland_nato"] | ||
scope: 'nato' |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sylius bundles never specify repository at
Configuration.php
I'm questioning this for years, but never actually research how and why it works this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's a custom repository then it's declared in the config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not true for 1.12, every instance that you've found was introduced in 1.13.
I still don't know why it was done this way and how it even work, but custom repositories are not specified for Configuration for any of the bundle