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-27428: Get translated API properties in the language requested #2002

Merged
merged 33 commits into from Jun 13, 2017

Conversation

3 participants
@andrerom
Member

andrerom commented May 31, 2017

Issue: https://jira.ez.no/browse/EZP-27428

Example

Instead of:

$content = $this->contentService->loadContent(
    42,
    $this->configResolver->getParameter('languages')
);

$name = $content->getVersionInfo()->getName(
    $this->configResolver->getParameter('languages')[0]
);
// OR rather to take care of all languages: $this->translationHelper->getTranslatedContentName($content);

$field = $this->translationHelper->getTranslatedField($content, 'body');
$value = $field->value;

We'll get:

$content = $this->contentService->loadContent(
    42,
    $this->configResolver->getParameter('languages')
);

$name = $content->getVersionInfo()->getName();
$value = $content->getFieldValue('body')

! However without breaking todays logic in TranslationHelper so existing code works as expected, which is why this can target master and not be limited to 7.0.

Reviewer notes:

  • You might want to review each commit separately here..
  • On Content and Version the prioritized field and name language is pre calculated in mapper, this is not strictly needed, it can be done like in the trait for generic name handling in https://github.com/ezsystems/ezpublish-kernel/compare/language_match_api_logic?expand=1#diff-bccf24da09e10e3f51c867eb7d751104R62
    • Reason for not pre-calculated there is same as not reusing pre-calculated prioriticedNameLanguageCode from VersionInfo also in Content: Name might exists in one translation where Field does not. Same goes for name vs description, one or the other might not be translated to a given language.
  • Followup on this is PR is #1966

Todo:

  • Object state (group) assuming the approach taken on content type (group) is ok
  • Add test coverage (currently none to make sure the change does not break anything in existing code)

@Nattfarinn Nattfarinn referenced this pull request May 31, 2017

Closed

New SiteAccessAware Repository layer #1966

0 of 3 tasks complete

@andrerom andrerom force-pushed the language_match_api_logic branch from c3b02a8 to a7e09e1 May 31, 2017

*
* Provides a uniform way for API consuming logic to generate translated description labels for API objects.
*/
abstract class MultiLanguageValueDescriptionBase extends MultiLanguageValueNameBase

This comment has been minimized.

@Nattfarinn

Nattfarinn Jun 1, 2017

Contributor

I don't like it.

  1. MultiLanguageValueDescriptionBase is not a child of MultiLanguageValueNameBase - not sure if business logic assumption that description always comes with name is a reason to implement inheritance.

  2. Both should be interfaces - their purpose is to ensure method signatures on descendants. Interfaces would also resolve multi-inheritance chaining issue.

abstract class ContentType extends ValueObject implements ValueDescriptionAware, ValueNameAware {}

vs.

abstract class MultiLanguageValueNameBase extends ValueObject {}
abstract class MultiLanguageValueDescriptionBase extends MultiLanguageValueNameBase {}
abstract class ContentType extends MultiLanguageValueDescriptionBase {}
  1. (ad. 1 + 2) If we need a place where we plan to add basic implementation in a future, maybe implementing base, empty abstract aggregating mentioned interfaces is a way to go.
abstract class MultiLanguageValueBase extends ValueObject implements ValueDescriptionAware, ValueNameAware {}
abstract class ContentType extends MultiLanguageValueBase {}
*/
public function getName($languageCode)
public function getName($languageCode = null)

This comment has been minimized.

@Nattfarinn

Nattfarinn Jun 1, 2017

Contributor

Method body will raise Notice in a default case.

This comment has been minimized.

@andrerom

andrerom Jun 1, 2017

Member

This is REST client, it is deeply broken anyway.

This comment has been minimized.

@andrerom

andrerom Jun 1, 2017

Member

But yes ,we should add todo's on these to make it more clear

This comment has been minimized.

@andrerom

andrerom Jun 6, 2017

Member

Solved now

* Provides a uniform way for API consuming logic to generate translated names / labels for API objects, language logic
* is meant to also be used for description, fields, .. lookup as well.
*/
abstract class MultiLanguageValueNameBase extends ValueObject

This comment has been minimized.

@Nattfarinn

Nattfarinn Jun 1, 2017

Contributor

(Same as "I don't like it." one)

/**
* @internal Meant for internal use by Repository, type hint against API object instead.
*/
trait MultiLanguageValueTrait

This comment has been minimized.

@Nattfarinn

Nattfarinn Jun 1, 2017

Contributor

Even if internal, wouldn't it be better to split it to two traits? (MultiLanguageValueNameTrait, MultiLanguageValueDescriptionTrait). Right now it seems to be rather more like an abstract base class than single responsibility trait.

This comment has been minimized.

@andrerom

andrerom Jun 1, 2017

Member

could do that, but you'd then duplicate a few of the params.

*
* @var string
*/
protected $mainLanguageCode;

This comment has been minimized.

@andrerom

andrerom Jun 6, 2017

Member

@alongosz why was this removed? :)

This comment has been minimized.

@alongosz

alongosz Jun 6, 2017

Member

@andrerom classes extending API ContentType redefine it via trait.
In PHP 5.6 redefining properties triggers E_STRICT error.

This comment has been minimized.

@andrerom

andrerom Jun 6, 2017

Member

ok, yeah I see it is not there on the other domain types which now uses these traits, so +1 then

* @param mixed $contentTypeGroupId
*
* @return \eZ\Publish\API\Repository\Values\ContentType\ContentTypeGroup
* {@inheritdoc}.

This comment has been minimized.

@alongosz

alongosz Jun 7, 2017

Member

A small nitpick on what I've just discovered. This dot after {@inheritdoc} is not needed (and php-cs-fixer is ok with that). When present it becomes a part of doc in the new line. Looks bad in PhpStorm.
Applies to several places in the PR.
inheritdoc_dot

This comment has been minimized.

@andrerom

andrerom Jun 9, 2017

Member

Ok, earlier php-cs-fixer was adding it. could you remove it?

@andrerom

This comment has been minimized.

Member

andrerom commented Jun 12, 2017

Ok, pulled in @alongosz's changes adapted and ready for review now @alongosz and @Nattfarinn

@alongosz

Looks good.
I meant to add more unit tests for other ValueObjects implementing language matching, as I did for ObjectState, but since we have tomorrow v6.10.0-beta1 I can add this later. Scenarios are covered by integration tests anyway.

@andrerom andrerom merged commit 36802d0 into master Jun 13, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
ezrobot/phpcsfixer Code review by ezrobot
Details

@andrerom andrerom deleted the language_match_api_logic branch Jun 13, 2017

@andrerom andrerom referenced this pull request Aug 4, 2017

Merged

EZP-27429: Optional SiteAccessAware Repository layer #2064

5 of 5 tasks complete
@andrerom

This comment has been minimized.

Member

andrerom commented on eZ/Publish/SPI/Repository/Values/MultiLanguageName.php in c300d1f Jan 18, 2018

@alongosz Didn't notice this before, but these seems to be wrongly placed in SPI, I assume they should have been in API as in SPI there is no repository. same with class above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment