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

EZP-32340: Deprecated ezpublish_rest.field_type_processor in favour of ibexa.rest.field_type.processor #63

Merged
merged 3 commits into from
Feb 19, 2021

Conversation

adamwojs
Copy link
Member

Question Answer
JIRA issue EZP-32340
Type improvement
Target version 3.2+
BC breaks no
Tests pass yes
Doc needed yes

Deprecated ezpublish_rest.field_type_processor in favour of ezplatform.field_type.rest.processor.

TODO:

  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@adamwojs adamwojs self-assigned this Feb 15, 2021
@adamwojs adamwojs requested a review from a team February 15, 2021 10:53
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Reference;

class FieldTypeProcessorPass implements CompilerPassInterface
{
public const FIELD_TYPE_PROCESSOR_SERVICE_TAG = 'ezplatform.field_type.rest.processor';
Copy link
Member

Choose a reason for hiding this comment

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

Since the tag was never really working we should go with new scheme to avoid adding to a pile of work we already have when it comes to cleanup.

Suggested change
public const FIELD_TYPE_PROCESSOR_SERVICE_TAG = 'ezplatform.field_type.rest.processor';
public const FIELD_TYPE_PROCESSOR_SERVICE_TAG = 'ibexa.rest.field_type.processor';

@adamwojs adamwojs changed the title EZP-32340: Deprecated ezpublish_rest.field_type_processor in favour of ezplatform.field_type.rest.processor EZP-32340: Deprecated ezpublish_rest.field_type_processor in favour of ibexa.rest.field_type.processor Feb 17, 2021
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Reference;

class FieldTypeProcessorPass implements CompilerPassInterface
{
public const FIELD_TYPE_PROCESSOR_SERVICE_TAG = 'ibexa.rest.field_type.processor';
public const DEPRECATED_FIELD_TYPE_PROCESSOR_SERVICE_TAG = 'ezpublish_rest.field_type_processor';
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's deprecated and nobody would have anywhere at this point, won't it make sense to make it private immediately?

Suggested change
public const DEPRECATED_FIELD_TYPE_PROCESSOR_SERVICE_TAG = 'ezpublish_rest.field_type_processor';
private const DEPRECATED_FIELD_TYPE_PROCESSOR_SERVICE_TAG = 'ezpublish_rest.field_type_processor';

We could use the constant above as !php/const in YAML files, but that's kinda nitpicky I guess?

Copy link
Member Author

Choose a reason for hiding this comment

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

The main purpose of this constants is to avoid hardcoding tag names in *.php (including tests), and only there 😉

Copy link
Contributor

@Steveb-p Steveb-p Feb 19, 2021

Choose a reason for hiding this comment

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

@adamwojs I'd say that the purpose of the tests is to detect a change in constant values in this case (since we expect users to use that tag). Similarly how hardcoding URLs for application tests is recommended.

If we're concerned that changing this tag will force us to change values in multiple files, then using !php/const makes more sense than in tests actually, as it's occuring there a lot more than in tests.

Copy link
Member Author

@adamwojs adamwojs Feb 19, 2021

Choose a reason for hiding this comment

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

Applied in #64

@bogusez bogusez self-assigned this Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants