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

Refactor FieldValueConverterRegistryPass not to use static callbacks #1863

Merged

Conversation

alongosz
Copy link
Member

@alongosz alongosz commented Dec 12, 2016

This PR refactors eZ\Publish\Core\Base\Container\Compiler\Storage\Legacy\FieldValueConverterRegistryPass not to use static callback when registering concrete FieldValueConverters.

This change simplifies injecting any dependency into any FieldValueConverter (and makes the code cleaner).

On the other hand, to compensate for "losing" laziness on Converters, Registry is now lazy.

TODO:

  • Remove static callback logic from the FieldValueConverterRegistryPass.
  • Deprecate static callback method create in concrete FieldValueConverter implementations.
  • Make ezpublish.persistence.legacy.field_value_converter.registry lazy
  • Remove lazy and callback attributes from ezpublish.storageEngine.legacy.converter tagged-services.
  • Align unit tests.
  • Remove TMP commit & rebase after Fix composer "memory exhausted" during Travis UnitTest preparation #1865 is merged - for Travis to work again.

*
* @return Author
*/
public static function create()
Copy link
Contributor

Choose a reason for hiding this comment

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

you should deprecate and not remove directly

Copy link
Contributor

@andrerom andrerom left a comment

Choose a reason for hiding this comment

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

Can't remove ::create directly.

@alongosz
Copy link
Member Author

Restored ::create and deprecated it in 99903c2.

@@ -22,11 +22,13 @@ class FloatConverter implements Converter
const HAS_MAX_VALUE = 2;

/**
* @deprecated since 6.8, will be removed in 7.x, use default constructor instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpic, but @deprecated is no the title of the doc block :)

so place it after "NOTE:" part.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, fixed in eb1b693 :)

Copy link
Contributor

@andrerom andrerom left a comment

Choose a reason for hiding this comment

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

Besides nitpick +1

@@ -5,6 +5,8 @@
*
* @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
Copy link
Contributor

Choose a reason for hiding this comment

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

missed this one now

* @return float
* @deprecated since 6.8, will be removed in 7.x, use default constructor instead.
*
* @return Float
Copy link
Contributor

Choose a reason for hiding this comment

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

and this

@alongosz
Copy link
Member Author

Added missing CS fixes in affa731 (sorry!).

@pspanja
Copy link
Contributor

pspanja commented Dec 13, 2016

From comment at #1812, should this be closed now?

@andrerom
Copy link
Contributor

andrerom commented Dec 13, 2016

From comment at #1812, should this be closed now?

Can, but this is a nice cleanup and already done, so would like to merge it for 6.8 if you approve it :)

Copy link
Contributor

@pspanja pspanja left a comment

Choose a reason for hiding this comment

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

Looks good, with two nitpicks fixed +1

* @return Float
* @deprecated since 6.8, will be removed in 7.x, use default constructor instead.
*
* @return float
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: FloatConverter :)

* @return Integer
* @deprecated since 6.8, will be removed in 7.x, use default constructor instead.
*
* @return int
Copy link
Contributor

Choose a reason for hiding this comment

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

IntegerConverter

@alongosz
Copy link
Member Author

Looks like almost every converter has wrong doc on create method return type. Since we're doing a cleanup I updated all of them in 548fede (using FQCN as we do everywhere).

@alongosz alongosz force-pushed the refactor-field-value-converters-pass branch from 17eaa0f to 8479987 Compare December 15, 2016 15:18
@alongosz
Copy link
Member Author

It's ready now.
I had to rebase this to be able to run composer on Travis again.
Since review is done I also squashed all commits.

// cc @andrerom

@andrerom andrerom merged commit 5e27a0c into ezsystems:master Dec 16, 2016
@andrerom
Copy link
Contributor

Thanks @alongosz

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.

3 participants