Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Rework the model collector #540

Merged
merged 21 commits into from
Feb 28, 2022
Merged

Rework the model collector #540

merged 21 commits into from
Feb 28, 2022

Conversation

baumannsven
Copy link
Member

Rework the model collector.

  • Allow the hierarchical has a parent data provider
  • Add new method for search the parent model from a hierarchical model
  • Add the missing search over the configured inverse filter

@baumannsven baumannsven added this to the 2.2.0 milestone Jul 18, 2020
@baumannsven baumannsven requested review from discordier and zonky2 July 18, 2020 11:02
@baumannsven baumannsven changed the base branch from master to release/2.2.0 July 18, 2020 11:02
discordier
discordier requested changes Aug 12, 2020
View reviewed changes
Copy link
Member

@discordier discordier left a comment

Choose a reason for hiding this comment

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

Added some remarks that need to be resolved first.

src/Controller/ModelCollector.php Outdated Show resolved Hide resolved
src/Controller/ModelCollector.php Outdated Show resolved Hide resolved
src/Controller/ModelCollector.php Outdated Show resolved Hide resolved
discordier
discordier requested changes Aug 12, 2020
View reviewed changes
Copy link
Member

@discordier discordier left a comment

Choose a reason for hiding this comment

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

Added some more remarks influenced by #541

src/Controller/ModelCollector.php Outdated Show resolved Hide resolved
src/Controller/ModelCollector.php Outdated Show resolved Hide resolved
src/Controller/ModelCollector.php Outdated Show resolved Hide resolved
@baumannsven baumannsven mentioned this pull request Aug 28, 2020
Open
@zonky2 zonky2 changed the title [RTM] Rework the model collector Rework the model collector Apr 15, 2021
dmolineus
dmolineus reviewed Nov 22, 2021
View reviewed changes
Copy link
Member

@dmolineus dmolineus left a comment

Choose a reason for hiding this comment

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

@discordier @baumannsven I reviewed this pull request, added some unit tests to understand the behaviour and changes some implementations based on discussions with @discordier.

src/Controller/ModelCollector.php Outdated Show resolved Hide resolved
src/Controller/ModelCollector.php Outdated Show resolved Hide resolved
src/Controller/ModelCollector.php Outdated Show resolved Hide resolved
@dmolineus dmolineus requested a review from discordier November 22, 2021 11:20
discordier
discordier requested changes Nov 30, 2021
View reviewed changes
Copy link
Member

@discordier discordier left a comment

Choose a reason for hiding this comment

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

Added some remarks.

Additionally, we discussed on Mumble that we want to have github actions in here before merging.

src/Controller/ModelCollector.php Outdated Show resolved Hide resolved
src/Controller/ModelCollector.php Outdated Show resolved Hide resolved
tests/Controller/ModelCollectorTest.php Outdated
use ContaoCommunityAlliance\DcGeneral\DataDefinition\ModelRelationship\RootConditionInterface;
use ContaoCommunityAlliance\DcGeneral\EnvironmentInterface;
use ContaoCommunityAlliance\DcGeneral\Exception\DcGeneralRuntimeException;
use ContaoCommunityAlliance\DcGeneral\Test\TestCase;
use Generator;
use function func_get_args;
Copy link
Member

Choose a reason for hiding this comment

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

PSR-12 requires an empty line here to separate blocks what does the current style say?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 7826ab3

tests/Controller/ModelCollectorTest.php Outdated
use ContaoCommunityAlliance\DcGeneral\DataDefinition\ModelRelationship\RootConditionInterface;
use ContaoCommunityAlliance\DcGeneral\EnvironmentInterface;
use ContaoCommunityAlliance\DcGeneral\Exception\DcGeneralRuntimeException;
use ContaoCommunityAlliance\DcGeneral\Test\TestCase;
use Generator;
use function func_get_args;
use function var_dump;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the forgotten debug import.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 7826ab3

tests/Controller/ModelCollectorTest.php Outdated

$collector = new ModelCollector($environment);

$this->assertSame($expected, $collector->searchParentOfIn($model, $candidates));
Copy link
Member

Choose a reason for hiding this comment

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

Please call static method via self::assertSame()

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 7826ab3

tests/Controller/ModelCollectorTest.php Outdated
);

$collector = new ModelCollector($environment);
$this->assertSame($expected, $collector->searchParentOfIn($model, $grandParents));
Copy link
Member

Choose a reason for hiding this comment

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

Please call static method via self::assertSame()

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 7826ab3

tests/Controller/ModelCollectorTest.php Outdated
);

$collector = new ModelCollector($environment);
$this->assertSame($expected, $collector->searchParentOf($model));
Copy link
Member

Choose a reason for hiding this comment

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

Please call static method via self::assertSame()

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 7826ab3

baumannsven and others added 12 commits February 21, 2022 16:00
@baumannsven @dmolineus
Rework the model collector.
95eca8f 
- Allow the hierarchical has a parent data provider
- Add new method for search the parent model from a hierarchical model
- Add the missing search over the configured inverse filter
@dmolineus
Add dynamically declared property $defaultProviderName
7b18734
@dmolineus
Add unit tests for ModelCollector::searchParentOfIn and remove condition
ec2b010
@dmolineus
Check the model provider in all search methods
c1e3ca4
@dmolineus
Guard that root and parent provider are defined when searching a parent
ec1f567
@dmolineus
Fix unit tests
4b3ef32
@dmolineus
Rename searchParentFromHierarchical to searchParentOfRootModel and de…
75d4faf 
…scribe the purpose
@dmolineus
Use the model collector instead of expecting that an inverse filter i…
b32e452 
…s defined
@dmolineus
Follow the interface ParentChildConditionInterface::getInverseFilterFor
d50031e
@dmolineus
Improve test method names
2ceecba
@dmolineus
Add unit test for hierachical search by inverse filter
9049596
@dmolineus
Fix code style
508256b
@dmolineus dmolineus force-pushed the feature/rework_model_collector branch from d7b4ed8 to ba51701 Compare February 21, 2022 15:08
@dmolineus
Copy link
Member

As discussed with @discordier I do not limit searchParentOf to a model of the default provider (459ee4f) anymore. As discussed with @baumannsven I dropped the searchParentOfRootModel method (5e99407). Feel free to add it in #541 again if really required.

@zonky2 zonky2 requested review from zonky2 and discordier February 23, 2022 16:14
zonky2
zonky2 approved these changes Feb 26, 2022
View reviewed changes
Copy link
Contributor

@zonky2 zonky2 left a comment

Choose a reason for hiding this comment

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

tested in MM 2.2 - works fine

I can delete attributes without error.

@zonky2 zonky2 mentioned this pull request Feb 26, 2022
Closed
@discordier discordier linked an issue Feb 28, 2022 that may be closed by this pull request
Error on delete MM attribute #538
Closed
@baumannsven
Copy link
Member Author

@dmolineus @discordier @zonky2

How is the state of this PR?

@zonky2
Copy link
Contributor

zonky2 commented Feb 28, 2022

wie schon geschrieben: @dmolineus ist fertig, @zonky2 hat Livetest gemacht und Anmerkungen von @discordier wurden eingepflegt...

@baumannsven baumannsven merged commit 5c240cb into release/2.2.0 Feb 28, 2022
@zonky2 zonky2 deleted the feature/rework_model_collector branch May 16, 2022 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmolineus dmolineus dmolineus left review comments

@zonky2 zonky2 zonky2 approved these changes

@discordier discordier Awaiting requested review from discordier

Assignees

@zonky2 zonky2

Labels
enhancement
Projects
None yet
Milestone
2.2.0
Development

Successfully merging this pull request may close these issues.

Error on delete MM attribute
4 participants
@baumannsven @dmolineus @zonky2 @discordier