Optimize matching zones to a given address #15275

jakubtobiasz · 2023-09-06T09:31:25Z

Q	A
Branch?	1.13
Bug fix?	performance fix
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	closes #9777
License	MIT

Test data:
~1000 zones <-> zone members 1:1
~20 zones created with up to 7 level of nesting (zone in the zone in the zone...)

Before:

After:

github-actions · 2023-09-06T09:43:07Z

Bunnyshell Preview Environment deleted

Available commands:

/bns:deploy to redeploy the environment

diimpp

I see it's a WIP, I'm just passing by 😅

src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php

src/Sylius/Component/Addressing/spec/Matcher/ZoneMatcherSpec.php

jakubtobiasz · 2023-09-15T17:12:36Z

src/Sylius/Bundle/AddressingBundle/test/var/db.sql

I have no idea how to make it working without the sqlite database file :|. If anyone has an idea – I encourage to share or directly contribute. Otherwise let's merge it and wait for new test apps.

🤷🏼‍♂️

src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php

src/Sylius/Bundle/AddressingBundle/Tests/Matcher/ZoneMatcherTest/fixtures.yaml

…ssQueryBuilder"

diimpp

Request changes is mostly for

--- $query
+++ $queryBuilder`

which I just noticed. (I didn't mark them all)

P.S. Sorry for the "waves" reviews, I hope next one be the last one.

diimpp · 2023-09-23T23:53:55Z

src/Sylius/Bundle/AddressingBundle/DependencyInjection/Configuration.php

@@ -142,7 +143,7 @@ private function addResourcesSection(ArrayNodeDefinition $node): void
                                        ->scalarNode('model')->defaultValue(Zone::class)->cannotBeEmpty()->end()
                                        ->scalarNode('interface')->defaultValue(ZoneInterface::class)->cannotBeEmpty()->end()
                                        ->scalarNode('controller')->defaultValue(ResourceController::class)->cannotBeEmpty()->end()
-                                        ->scalarNode('repository')->cannotBeEmpty()->end()
+                                        ->scalarNode('repository')->defaultValue(ZoneRepository::class)->cannotBeEmpty()->end()


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.

If there's a custom repository then it's declared in the config

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

src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php

Changes have been applied

GSadee · 2023-09-27T10:08:18Z

Thanks, Jacob! 🎉

NoResponseMate · 2023-09-29T11:34:14Z

src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php

+/**
+ * @implements ZoneRepositoryInterface<ZoneInterface>
+ */
+class ZoneRepository extends EntityRepository implements ZoneRepositoryInterface


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.

NoResponseMate · 2023-09-29T11:36:20Z

src/Sylius/Component/Addressing/Matcher/ZoneMatcher.php

-    ];
-
-    public function __construct(private RepositoryInterface $zoneRepository)
+    public function __construct(private ZoneRepositoryInterface $zoneRepository)


Should also be noted in the upgrade file

This PR was merged into the 1.13 branch. Discussion ---------- | Q | A | |-----------------|--------------------------------------------------------------| | Branch? | 1.13  | | Bug fix? | yes | | New feature? | no | | BC breaks? | not anymore | | Related tickets | #15275 (comment) | | License | MIT | Commits ------- [Upgrade] Note ZoneRepository addition and ZoneMatcher constructor changes [Addressing] Support BC for ZoneMatcher [ECS] Apply ecs fixes

This PR was merged into the 1.13 branch. Discussion ---------- | Q | A | |-----------------|--------------------------------------------------------------| | Branch? | 1.13  | | Bug fix? | yes | | New feature? | no | | BC breaks? | not anymore | | Related tickets | Sylius/Sylius#15275 (comment) | | License | MIT | Commits ------- [Upgrade] Note ZoneRepository addition and ZoneMatcher constructor changes [Addressing] Support BC for ZoneMatcher [ECS] Apply ecs fixes

jakubtobiasz added the Performance label Sep 6, 2023

jakubtobiasz requested a review from a team as a code owner September 6, 2023 09:31

GSadee previously approved these changes Sep 8, 2023

View reviewed changes

jakubtobiasz dismissed GSadee’s stale review via 2a21598 September 12, 2023 06:57

jakubtobiasz changed the base branch from 1.12 to 1.13 September 12, 2023 07:02

jakubtobiasz changed the title ~~Add eager loading for Zone's members~~ Optimize matching zones to a given address Sep 12, 2023

jakubtobiasz added 8 commits September 12, 2023 11:33

Add eager loading for Zone's members

84aaf55

Add a method finding all zones by address to the Zone's repository

ba06fc1

Add a method finding all zones by a zone to the Zone's repository

a69fb7a

Implement more optimized ZoneMatcher::matchAll method

8b6c365

Implement more optimized ZoneMatcher::match method

660c95d

Provide ECS fixes

43004fe

Move ZoneRepositoryInterface from the Bundle to the Component

5c6033d

Take into account 'all' scope in the Zone

7491c61

jakubtobiasz force-pushed the SYL-2705 branch from c77771e to 6d55f85 Compare September 12, 2023 09:34

Add AliceDataFixturesBundle to the test app in the AddressingBundle

5a3923a

jakubtobiasz force-pushed the SYL-2705 branch from 6d55f85 to 5a3923a Compare September 12, 2023 09:46

jakubtobiasz changed the title ~~Optimize matching zones to a given address~~ 🚧 Optimize matching zones to a given address Sep 12, 2023

diimpp reviewed Sep 14, 2023

View reviewed changes

jakubtobiasz added 4 commits September 15, 2023 16:13

Add selecting zone members to prevent lazy loading them

3287b8c

Add purging the test database between phpunit tests

7bf31b2

Remove the visibility definitions for methods in the spec

830c741

Make the createFindByAddressQueryBuilder public

c152bb5

jakubtobiasz changed the title ~~🚧 Optimize matching zones to a given address~~ Optimize matching zones to a given address Sep 15, 2023

jakubtobiasz force-pushed the SYL-2705 branch from d942eb3 to 66a1b99 Compare September 15, 2023 15:35

Replace overriding the zones argument with a new zonesCodes variable

7e975d3

jakubtobiasz force-pushed the SYL-2705 branch from 66a1b99 to 7e975d3 Compare September 15, 2023 15:39

Fix database-based tests

bea9452

jakubtobiasz commented Sep 15, 2023

View reviewed changes

GSadee reviewed Sep 21, 2023

View reviewed changes

jakubtobiasz added 4 commits September 21, 2023 08:59

Remove the unnecessary eager loading of zone's members

6e5bcee

Rename the "findZonesByMembers" method to "findByMembers"

68a47ca

Rename the "findAllByAddress" method to "findByAddress"

66cfe9c

Rename the "createFindByAddressQueryBuilder" method to "createByAddre…

8922d7e

…ssQueryBuilder"

jakubtobiasz force-pushed the SYL-2705 branch 2 times, most recently from fabef59 to 2a47288 Compare September 21, 2023 10:31

jakubtobiasz added 2 commits September 21, 2023 12:37

Adjust scopes' names to better reflect Sylius' zone scopes

fc261db

Adjust tables' aliases

89a5e8a

jakubtobiasz force-pushed the SYL-2705 branch from 2a47288 to 89a5e8a Compare September 21, 2023 10:38

Replace the findOneByAddress method with findOneByAddressAndType

8365fbc

NoResponseMate previously approved these changes Sep 22, 2023

View reviewed changes

diimpp previously requested changes Sep 24, 2023

View reviewed changes

Rename $query variables to $queryBuilder

d72ffda

jakubtobiasz dismissed NoResponseMate’s stale review via d72ffda September 27, 2023 05:36

TheMilek approved these changes Sep 27, 2023

View reviewed changes

Rafikooo approved these changes Sep 27, 2023

View reviewed changes

Prometee approved these changes Sep 27, 2023

View reviewed changes

GSadee approved these changes Sep 27, 2023

View reviewed changes

GSadee merged commit 3dd0110 into Sylius:1.13 Sep 27, 2023
25 checks passed

diimpp mentioned this pull request Sep 29, 2023

[Issue 15271] - Get product variant by id #15273

Merged

NoResponseMate reviewed Sep 29, 2023

View reviewed changes

jakubtobiasz deleted the SYL-2705 branch October 12, 2023 12:13

jakubtobiasz mentioned this pull request Nov 10, 2023

Introduce Repository classes for Addressing, Attribute& Currency #15503

Merged

Rafikooo mentioned this pull request Jan 17, 2024

Fix ZoneRepository BC-Break #15739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize matching zones to a given address #15275

Optimize matching zones to a given address #15275

jakubtobiasz commented Sep 6, 2023 •

edited

github-actions bot commented Sep 6, 2023 •

edited

diimpp left a comment

jakubtobiasz Sep 15, 2023

jakubtobiasz Sep 15, 2023

diimpp left a comment •

edited

diimpp Sep 23, 2023

jakubtobiasz Sep 25, 2023

diimpp Sep 27, 2023 •

edited

GSadee commented Sep 27, 2023

NoResponseMate Sep 29, 2023

NoResponseMate Sep 29, 2023

Optimize matching zones to a given address #15275

Optimize matching zones to a given address #15275

Conversation

jakubtobiasz commented Sep 6, 2023 • edited

github-actions bot commented Sep 6, 2023 • edited

Bunnyshell Preview Environment deleted

diimpp left a comment

Choose a reason for hiding this comment

jakubtobiasz Sep 15, 2023

Choose a reason for hiding this comment

jakubtobiasz Sep 15, 2023

Choose a reason for hiding this comment

diimpp left a comment • edited

Choose a reason for hiding this comment

diimpp Sep 23, 2023

Choose a reason for hiding this comment

jakubtobiasz Sep 25, 2023

Choose a reason for hiding this comment

diimpp Sep 27, 2023 • edited

Choose a reason for hiding this comment

GSadee commented Sep 27, 2023

NoResponseMate Sep 29, 2023

Choose a reason for hiding this comment

NoResponseMate Sep 29, 2023

Choose a reason for hiding this comment

jakubtobiasz commented Sep 6, 2023 •

edited

github-actions bot commented Sep 6, 2023 •

edited

diimpp left a comment •

edited

diimpp Sep 27, 2023 •

edited