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-27417: As a Developer I want API to allow removal of the translation from all content object versions #2031

Merged
merged 14 commits into from Jul 13, 2017

Conversation

4 participants
@alongosz
Copy link
Member

alongosz commented Jun 22, 2017

Implements EZP-27417

This PR implements ContentService::removeTranslation(ContentInfo, languageCode) method.
The method removes a translation specified by the languageCode from all the Versions of a Content Object (including archived ones).

TODO:

  • Implement Core ContentService::removeTranslation method.
  • Move proposed specification to permanent place in doc/.
  • Add removeTranslation method to the ContentService interface.
  • Add Core RemoveTranslationSignal
  • Implement SignalSlot\ContentService::removeTranslation emitting RemoveTranslationSignal.
  • Align Core REST Client ContentService with interface changes (w/o implementing as it's been done before)
  • Add unit test for SignalSlot\ContentService.
  • Add integration tests for ContentService::removeTranslation method
  • Update versionInfo->initialLanguageCode with contentInfo->mainLanguageCode if the removed translation is of a former one.

@alongosz alongosz force-pushed the alongosz:ezp-27417-all-versions-translation-removal branch 3 times, most recently from c4e56f9 to e668467 Jun 26, 2017

@alongosz alongosz changed the title [WIP] EZP-27417: As a Developer I want API to allow removal of the translation from all content object versions EZP-27417: As a Developer I want API to allow removal of the translation from all content object versions Jun 26, 2017

@alongosz
Copy link
Member Author

alongosz left a comment

Ready for a review, as usual some notes on decisions I made:

@@ -14,7 +14,7 @@ the existing Versions of a Content Object and is specified as:
* a warning before performing it.
*
* @throws \eZ\Publish\API\Repository\Exceptions\BadStateException if the specified translation
* is the only one Content Object has.
* is the only one Content Object has or it is the main translation.

This comment has been minimized.

@alongosz

alongosz Jun 26, 2017

Author Member

I think we forgot to discuss & include the case where Content Object Version has more than one translation, but the one being removed is the main translation (contentInfo->mainLanguageCode, versionInfo->initialLanguageCode). IMHO there is no easy recovery from such situation, so for now I disallowed it throwing BadStateException.

It also "covers" the case where a translation is the only one, because the only one translation is the main one (which is reflected in the integration tests PhpDoc).

// cc @andrerom

This comment has been minimized.

@andrerom

andrerom Jul 5, 2017

Member

A couple of questions on this (ref: https://jira.ez.no/browse/EZP-27538):

  1. Removing main translation is disallowed, but there is api to change main so this won't be a blocker right?
  2. How do we handle initial language for versions? As it can be different one from version to version we will need to pick another one being initial version.
  3. We will also need to clarify what we do with drafts, they typically only contain one language, so either removing the translation involves removing the draft which I guess should throw.
  4. with all the different cases of throws here I guess we should model this better to clearly tell what is wrong.

This comment has been minimized.

@andrerom

andrerom Jul 5, 2017

Member

On the initial version I might be wrong, but afaik the current proposed logic means it won't allow you to delete any language, as typically there will be one version for each translation added where that translation is the initial one.

This comment has been minimized.

@alongosz

alongosz Jul 5, 2017

Author Member

Removing main translation is disallowed, but there is api to change main so this won't be a blocker right?

Exactly. To remove translation which is the main one, you have to update mainLanguageCode content metadata. I should probably add some integration test proving that... ;)

How do we handle initial language for versions? As it can be different one from version to version we will need to pick another one being initial version.

On the initial version I might be wrong, but afaik the current proposed logic means it won't allow you to delete any language, as typically there will be one version for each translation added where that translation is the initial one.

I actually haven't seen this case when creating tests, but I would imagine it's quite probable.
My thought on this is then - in such case, let's update initialLanguageCode with mainLanguageCode.

We will also need to clarify what we do with drafts, they typically only contain one language, so either removing the translation involves removing the draft which I guess should throw.

I wouldn't complicate removal API. I'd rather state in the doc, that you have to remove manually any version that has only one language.

with all the different cases of throws here I guess we should model this better to clearly tell what is wrong.

On that I would need some help as I'm not sure what you're aiming here for. Ideally this probably should be different exception, but BadStateException really fits both cases. I ensured it throws different message. However, as I stated in previous comment, exception related to a translation being the only one shouldn't happen now, as the one about mainLanguageCode happens sooner.
So, maybe I'll just drop "only one translation" check/exception?

This comment has been minimized.

@andrerom

andrerom Jul 6, 2017

Member

for the first points 👍

for the last one on exceptions, afaik we have:

  • main
  • draft with language (as it would imply removing the draft)
  • just one translation

right?

This comment has been minimized.

@andrerom

andrerom Jul 6, 2017

Member

I think if we have "just one translation" it also implies "main", right?

true

as for your other comment I'll need to refresh my mind on the translation handling and drafts it seems.

This comment has been minimized.

@andrerom

andrerom Jul 6, 2017

Member

As for exception, lets stay with BadState for now, we can always extend it later and provide more specific exception structure here if needed.

All good @alongosz ?
You update the phpdoc and code to reflect this?

This comment has been minimized.

@alongosz

alongosz Jul 6, 2017

Author Member

Ok, then it seems that all what's left is updating initialLanguageCode.

This comment has been minimized.

@andrerom

andrerom Jul 6, 2017

Member

well, update phpdoc here on top, drop the exception on initial language check, and as you say add logic to update initial language if we remove it. right?

This comment has been minimized.

@alongosz

alongosz Jul 7, 2017

Author Member

PhpDoc in Spec updated in 050d76d.

$content = $this->createContentVersion2();

// delete first version which has only one translation
$contentService->deleteVersion($contentService->loadVersionInfo($content->contentInfo, 1));

This comment has been minimized.

@alongosz

alongosz Jun 26, 2017

Author Member

Note: As mentioned above, this doesn't change anything - I just want to reflect separate use cases.

$query = $this->connection->createQueryBuilder();
$query->update('ezcontentobject')
// parameter for bitwise operation has to be placed verbatim (w/o binding) for this to work cross-DBMS
->set('language_mask', 'language_mask & ~ ' . $languageId)

This comment has been minimized.

@alongosz

alongosz Jun 26, 2017

Author Member

Note: A little bit of bitwise voodoo that seems to be working for MySQL, Sqlite and PostgreSQL (thx @Nattfarinn :) ).
I didn't bind it as a parameter because PostgreSQL cannot determine data type then and demands explicit cast... which is DBMS-specific.
Anyway, $languageId comes from $this->languageHandler->loadByLanguageCode($languageCode); so we can safely assume it's integer.

$query->expr()->orX(
'language_mask & ~ ' . $languageId . ' <> 0',
'language_mask & ~ ' . $languageId . ' <> 1'
)

This comment has been minimized.

@alongosz

alongosz Jun 26, 2017

Author Member

Note: More bitwise voodoo. If the above operation gives us either 0 or 1 this means the language was the last one (1 - alwaysAvailable). There's a small risk of race between validation in Core ContentService::removeTranslation and DBMS transaction, so I placed this condition just to be safe.

@alongosz alongosz requested review from andrerom and Nattfarinn Jun 26, 2017

@alongosz alongosz force-pushed the alongosz:ezp-27417-all-versions-translation-removal branch from e668467 to 16b20c3 Jun 30, 2017

@@ -218,9 +217,9 @@ protected function fetchCurrentVersionUrl($currentVersionReference)
*/
protected function isErrorResponse(Message $response)
{
return (
return
strpos($response->headers['Content-Type'], 'application/vnd.ez.api.ErrorMessage') === 0

This comment has been minimized.

@andrerom

andrerom Jul 5, 2017

Member

same line as return now?

This comment has been minimized.

@alongosz

alongosz Jul 5, 2017

Author Member

Fixed in cb209c8.

@bdunogier

This comment has been minimized.

Copy link
Member

bdunogier commented Jul 5, 2017

Do we know when this may be merged ? We need it for https://jira.ez.no/browse/EZP-27538.

null,
1,
RemoveTranslationSignal::class,
array('contentId' => $contentId, 'languageCode' => $language),

This comment has been minimized.

@andrerom

andrerom Jul 6, 2017

Member

[] :P

This comment has been minimized.

@alongosz

alongosz Jul 6, 2017

Author Member

I left this on purpose, because it would imply changing this entire block and as discussed it "obfuscates" the diff :)

So I see two solutions:

  1. I still can do it only for this one-line inner array. It actually doesn't look that bad.
  2. I change whole block in a separate commit and we try to live with this :)

The second solution I applied somewhere before, so I know, I'm already inconsistent ;)

POV ping @Nattfarinn @andrerom

This comment has been minimized.

@andrerom

andrerom Jul 6, 2017

Member

ok clear.

Well either we then go with this, or 1, I'm ok with both.

This comment has been minimized.

@alongosz

alongosz Jul 6, 2017

Author Member

ok, let's keep it as is for now.

@alongosz alongosz force-pushed the alongosz:ezp-27417-all-versions-translation-removal branch from 050d76d to c6fe61f Jul 7, 2017

@alongosz

This comment has been minimized.

Copy link
Member Author

alongosz commented Jul 7, 2017

I left fixups here for new code review.

Most important changes:

  1. I was able to create test case where there is only one, non-main translation in one of the Versions. This test and other integration tests changes are in b001b76.
  2. Due to 1. my previous fix on Spec is invalid, in 3ddd806 there's more detailed one.
  3. Changes to implementation are available in c6fe61f (and covered in mentioned above integration test changes)
  4. Above changes include mostly updating initialLanguageCode of a Version and are covered in the integration test mentioned above.
  5. While testing update to initialLanguageCode I found a bug with loadContent returning not updated old data. Fixed here and improved integration tests.
);
$this->cache->clear('content', $contentId);
$this->cache->clear('content', 'info', $contentId);

This comment has been minimized.

@andrerom

andrerom Jul 10, 2017

Member

Convention is to do this after the persistence call, not before.

This comment has been minimized.

@alongosz

alongosz Jul 10, 2017

Author Member

Fixed in 735091d

*
* @param int $contentId
* @param string $languageCode language code of the translation
* @param string $mainLanguageCode Content Object main language code

This comment has been minimized.

@andrerom

andrerom Jul 10, 2017

Member

From puristic point of view this should not be here, as Persistence has enough to figure it out. (gateway can still have it)

But in case you disagree, we will at least need to better document this so we make it clear it HAS TO be the correct main language, this is not for changing main and that will lead to strange behavior if it is not the right one.

This comment has been minimized.

@alongosz

alongosz Jul 10, 2017

Author Member

Kind of agree, I wanted to avoid loading it once again in the persistence layer.
Removed this parameter in 4f81f18.
In ef783ff, there's an example how we can do it using nested select in update query.

@alongosz alongosz force-pushed the alongosz:ezp-27417-all-versions-translation-removal branch from c6fe61f to ef783ff Jul 10, 2017

// update initial_language_id only if it matches removed translation languageId
->set(
'initial_language_id',
'CASE WHEN initial_language_id = :languageId THEN (' . $mainLanguageSelectQuery->getSQL() . ') ELSE initial_language_id END'

This comment has been minimized.

@alongosz

alongosz Jul 10, 2017

Author Member

Follow-up on this comment:

This concatenation and call to ->getSQL() is rather ugly, but not as bad as verbatim nested select ;)
I would love this subquery to be bound as a parameter but it seems it's not possible with Doctrine\DBAL.

Overall I don't think this whole solution is as ugly as I expected, but if you want I can fetch main language separately.
// cc @andrerom, @Nattfarinn

This comment has been minimized.

@andrerom

andrerom Jul 11, 2017

Member

Would it be possible to do like in copyRelations() a few methods above here? Imo that approach would be cleaner when we need to do things like this :)

This comment has been minimized.

@alongosz

alongosz Jul 11, 2017

Author Member

Would it be possible to do like in copyRelations() a few methods above here? Imo that approach would be cleaner when we need to do things like this :)

Ok, I've discussed this w/ @Nattfarinn and we've agreed that this should be done in the same way. So, with better indentation for readability, amended in 2a3a400.

@alongosz alongosz force-pushed the alongosz:ezp-27417-all-versions-translation-removal branch from ef783ff to 2a3a400 Jul 11, 2017

->set(
'initial_language_id',
'CASE WHEN initial_language_id = :languageId ' .
'THEN (SELECT initial_language_id FROM ezcontentobject c WHERE c.id = :contentId) ' .

This comment has been minimized.

@andrerom

andrerom Jul 12, 2017

Member

Nitpick: Could perhaps set alias for initial_language_id to main_language_id if that is the case for reader.

This comment has been minimized.

@alongosz

alongosz Jul 12, 2017

Author Member

Meaning SELECT initial_language_id AS main_language_id FROM... as fixed in a155f03?

@andrerom
Copy link
Member

andrerom left a comment

👍 but be aware there are two missing followups for RemoveTranslationSignal:

review ping @Nattfarinn

@alongosz

This comment has been minimized.

Copy link
Member Author

alongosz commented Jul 12, 2017

be aware there are two missing followups for RemoveTranslationSignal:

Ok. We specified in the Spec doc that reindexing and http cache clearing should be done separately, so to avoid blocking this one, let's do a separate improvement or story maybe?

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jul 12, 2017

@alongosz that was what I was implying (seperate story)

@Nattfarinn

This comment has been minimized.

Copy link
Contributor

Nattfarinn commented Jul 13, 2017

Only thing I'm not sure about is {@inheritdoc} thing, but I guess we decided that using it is not an issue.

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jul 13, 2017

Only thing I'm not sure about is {@inheritdoc} thing, but I guess we decided that using it is not an issue.

I think it is ok for now, PSR-5 efforts just got restarted yesterday, so maybe we need to adjust to @inheritdoc block tag if that get in, but that would be very simple change.

@andrerom andrerom merged commit f0ec2b9 into ezsystems:master Jul 13, 2017

3 checks passed

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

@alongosz alongosz deleted the alongosz:ezp-27417-all-versions-translation-removal branch Nov 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.