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.

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

Fix setting current user to multiple Repositories #67

Closed
wants to merge 4 commits into from

Conversation

pspanja
Copy link
Member

@pspanja pspanja commented Jan 25, 2018

Fixes #64

This fixes the problem of setting current Repository user, where Repository instance used to get SearchService with Legacy Search Engine (used for FilterService) was not updated. Current User is the only state Core Repository keeps.

The problem is fixed with new Aggregate Repository implementation, which is registered as public Repository (it rewrites ezpublish.api.repository alias). This implementation aggregates top eZ Platform Repository implementation (default ezpublish.api.repository alias) and our custom instance and updates current user on all of them.

@emodric
Copy link
Member

emodric commented Jan 25, 2018

I'll look at it soon.

For now, only a coment that this should probably go against 2.1 branch. That's where filter service was introduced.

@pspanja
Copy link
Member Author

pspanja commented Jan 25, 2018

Agreed, this is to master for testing only.

@MarioBlazek
Copy link
Contributor

I can confirm, it does solve issue in my case.

@pspanja
Copy link
Member Author

pspanja commented Jan 25, 2018

BTW, if someone already implements a custom Repository layer, it would have to be done like here, by replacing ezpublish.api.repository alias. Not tested, but in that case this should work transparently with it.

@emodric
Copy link
Member

emodric commented Jan 26, 2018

I realize that there is no sane way to do this, but just for the record, I'm not sure I like replacing the ezpublish.api.repository alias. It feels too much invasive and I think it increases the chances of something breaking later on.

That being said, if we're going to go this way, it would be great if we could add custom repositories to the agreggate one with tags, so potential custom repositories can be added to the aggregate automatically, without having to modify the aggregate repo service definition.

Other than that, this looks good 👍

@pspanja
Copy link
Member Author

pspanja commented Jan 29, 2018

@emodric

I realize that there is no sane way to do this, but just for the record, I'm not sure I like replacing the ezpublish.api.repository alias. It feels too much invasive and I think it increases the chances of something breaking later on.

Actually, that should be possible, one should be able to implement own API layer and AFAIK this would be the way to do it. The weird part is second instance of Repository, but we had that before. And probably will have it until ezpublish-kernel implementation is decoupled so that it becomes possible to build SearchService in a sane way.

#
netgen.ezplatform_site.aggregate_repository:
class: Netgen\EzPlatformSiteApi\Core\Repository\Aggregate\Repository
arguments:
Copy link
Member

Choose a reason for hiding this comment

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

This should be a private service by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in a822caa.

@pspanja
Copy link
Member Author

pspanja commented Feb 2, 2018

@emodric implemented custom repo registration through service tag in f7a8dee

@MarioBlazek please test

@@ -38,10 +38,23 @@ class Repository implements RepositoryInterface
*/
public function __construct(
RepositoryInterface $ezRepository,
array $customRepositories
array $customRepositories = []
Copy link
Member

Choose a reason for hiding this comment

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

Do we now need a second param here?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't use it here, I just always add it to aggregate implementation of something.

Copy link
Member

Choose a reason for hiding this comment

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

Then you don't need addRepository method, since you can use $definition->replaceArgument(1, [...]) in the compiler pass, where [...] is an array of Reference objects.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, updated in 4591139

@emodric
Copy link
Member

emodric commented Feb 2, 2018

👍, with rebase to 2.1-release

@pspanja pspanja changed the base branch from master to 2.1-release February 2, 2018 12:48
@pspanja pspanja changed the base branch from 2.1-release to master February 2, 2018 13:19
@pspanja
Copy link
Member Author

pspanja commented Feb 2, 2018

Thanks guys, merged manually.

@pspanja pspanja closed this Feb 2, 2018
@pspanja pspanja deleted the repository-fix branch February 2, 2018 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent results
3 participants