Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

EZP-30693: Refactored SolrEngineFactory to avoid using container directly #143

Merged
merged 9 commits into from
Jul 1, 2019

Conversation

webhdx
Copy link
Contributor

@webhdx webhdx commented Jun 25, 2019

JIRA: https://jira.ez.no/browse/EZP-30693

Introduced registries and service tagging to allow fetching Gateways and CoreFilters for given connection. This avoid direct Container::get calls and let us keep services private.

@webhdx webhdx added the Bug label Jun 25, 2019
@webhdx webhdx self-assigned this Jun 25, 2019
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Nitpick:

* @copyright Copyright (C) eZ Systems AS. All rights reserved.
* @license For full copyright and license information view LICENSE file distributed with this source code.
*
* @version //autogentag//
Copy link
Member

Choose a reason for hiding this comment

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

It's been obsolete for a very long time, please skip it in all new files

Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

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

What about unit tests for introduced/modified code?

*/
public function process(ContainerBuilder $container)
{
if (!$container->hasDefinition('ezpublish.search.solr.core_filter.registry')) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please extract constant with registry service id?


$coreFilterRegistryDefinition = $container->getDefinition('ezpublish.search.solr.core_filter.registry');

$coreFilters = $container->findTaggedServiceIds('ezpublish.search.solr.core_filter');
Copy link
Member

Choose a reason for hiding this comment

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

Could you please extract constant with tag name?

*/
public function process(ContainerBuilder $container)
{
if (!$container->hasDefinition('ezpublish.search.solr.gateway.registry')) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please extract constant with gateway service id?


$gatewayRegistryDefinition = $container->getDefinition('ezpublish.search.solr.gateway.registry');

$gateways = $container->findTaggedServiceIds('ezpublish.search.solr.gateway');
Copy link
Member

Choose a reason for hiding this comment

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

Could you please extract constant with tag name?

use Symfony\Component\DependencyInjection\Reference;
use LogicException;

class CoreFilterRegistryPass implements CompilerPassInterface
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class CoreFilterRegistryPass implements CompilerPassInterface
final class CoreFilterRegistryPass implements CompilerPassInterface

use Symfony\Component\DependencyInjection\Reference;
use LogicException;

class GatewayRegistryPass implements CompilerPassInterface
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class GatewayRegistryPass implements CompilerPassInterface
final class GatewayRegistryPass implements CompilerPassInterface

use EzSystems\EzPlatformSolrSearchEngine\CoreFilter;
use OutOfBoundsException;

class CoreFilterRegistry
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class CoreFilterRegistry
final class CoreFilterRegistry

/**
* Registry for Solr search engine coreFilters.
*/
class GatewayRegistry
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class GatewayRegistry
final class GatewayRegistry

@webhdx
Copy link
Contributor Author

webhdx commented Jun 26, 2019

@adamwojs We are still looking for a volunteer. :)

@Nattfarinn
Copy link
Contributor

We're run out of juniors and interns :P

@webhdx
Copy link
Contributor Author

webhdx commented Jun 26, 2019

@adamwojs What exactly do you want to test? I'm fine with Compiler Passes but there is little sense in testing Registries as they only contain getters and setters.

@adamwojs
Copy link
Member

@adamwojs What exactly do you want to test? I'm fine with Compiler Passes but there is little sense in testing Registries as they only contain getters and setters.

Tests for Compiler Passes should be enough. Personally, I think registries are also worth to cover in tests: there are still services, even if it's logic is very simple. But this is my personal opinion.

BTW. Failed CI build seems to be relevant 😉

@webhdx
Copy link
Contributor Author

webhdx commented Jun 26, 2019

@adamwojs Added tests for Registries and CompilerPasses. Travis should be green now.

@adamwojs
Copy link
Member

@adamwojs Added tests for Registries and CompilerPasses. Travis should be green now.

Thank you @webhdx 🍻

Copy link
Member

@mnocon mnocon left a comment

Choose a reason for hiding this comment

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

Works now! 🎉

Copy link

@m-tyrala m-tyrala left a comment

Choose a reason for hiding this comment

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

QA approve

@lserwatka lserwatka merged commit cd12d9c into master Jul 1, 2019
@alongosz alongosz deleted the gateway_and_core_filter_refactoring branch July 17, 2020 09:58
# for free to join this conversation on GitHub. Already have an account? # to comment
Development

Successfully merging this pull request may close these issues.

7 participants