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

[Spec] EZP-27417: As a Developer I want API to allow removal of the translation from all content object versions #2001

Merged
merged 1 commit into from Jun 20, 2017

Conversation

7 participants
@alongosz
Copy link
Member

alongosz commented May 31, 2017

Proposed specification for EZP-27417

This PR specifies proposed PHP API changes which would allow removal of the translation from all the Content Object Versions.

Right now spec covers two one use case:
1. Creating a new version (draft) of content object with the translation removed.
2. Removing at once all the translations from all existing versions (with suggested usage).

The first use case got separated into another Story: EZP-27508

TODO:

  • Add specification for PHP API on removal of the content object translations.

@alongosz alongosz force-pushed the alongosz:ezp-27417-translation-removal-spec branch from d180c3c to 5fc948d Jun 1, 2017

@alongosz alongosz changed the title [WIP][Spec] EZP-27417: As a Developer I want API to allow removal of single translation w/o deleting the whole object [Spec] EZP-27417: As a Developer I want API to allow removal of single translation w/o deleting the whole object Jun 1, 2017

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

```

The method removes specific translation from all the existing versions of a content object.
Search engine reindexing has to be done manually after performing this operation.

This comment has been minimized.

@andrerom

andrerom Jun 1, 2017

Member

Search engine + HttpCache

But you should add until SearchEngine and HttpCache has been gotten support for corresponding PurgeTranslationSignal, afaik solr and http cache should get enough info in a signal to be able to react. As for LegacySearchEngine, you might best know how it should react :)

Both methods should have Signals, btw, you can pherhaps udpate spec for that and propose how signal should look like as part of this spec.

* @param string $languageCode
*
*/
public function purgeTranslations(ContentInfo $contentInfo, $languageCode);

This comment has been minimized.

@andrerom

andrerom Jun 1, 2017

Member

purgeTranslations => purgeTranslation

This comment has been minimized.

@andrerom

andrerom Jun 1, 2017

Member

Also naming... Maybe, unsure.. There is no use of the word purge in the API. So more consistent would be delete or remove, unless you see this as a different case then the other maybe?

## Removing specific language translation from all the content object versions

For the cases of system maintenance involving e.g. removing a language, PHP API introduces
the `purgeTranslations` method on `ContentService`, specified as:

This comment has been minimized.

@andrerom

andrerom Jun 1, 2017

Member

-s

*
* @throws \eZ\Publish\API\Repository\Exceptions\BadStateException if the specified translation
* is the only one content object has.
*

This comment has been minimized.

@andrerom

andrerom Jun 1, 2017

Member

you should add on both:
* @throws \eZ\Publish\API\Repository\Exceptions\UnauthorizedException if the user is not allowed to delete the content (in one of the locations of the given content object)

* @param \eZ\Publish\API\Repository\Values\Content\VersionInfo $versionInfo
* @param string $languageCode
*
* @return \eZ\Publish\API\Repository\Values\Content\Content Content draft

This comment has been minimized.

@andrerom

andrerom Jun 1, 2017

Member

* @return \eZ\Publish\API\Repository\Values\Content\Content - the newly created content draft

*
* @return \eZ\Publish\API\Repository\Values\Content\Content Content draft
*/
public function deleteTranslation(VersionInfo $versionInfo, $languageCode);

This comment has been minimized.

@andrerom

andrerom Jun 1, 2017

Member

naming.. this method actually does not delete anything, so maybe it's more of a
createContentDraftWithouthTranslation() But yeah, long, and it would suggests it should take contentInfo as argument which we don't really care about here.

Suggestions @Nattfarinn ? :)

@dfritschy

This comment has been minimized.

Copy link
Contributor

dfritschy commented Jun 15, 2017

What is the exact use case where you would want a new content draft with the language removed? The legacy eZContentObject::removeTranslation() simply removes the translation from all versions. IMHO this is sufficient.

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jun 15, 2017

@dfritschy which is covered by purgeTranslations() right?

you might be right, deleteTranslation() is rather easy to do on top of API today already so might not be needed. The idea was that this is a safer way to delete translation in use cases where you actually want to keep history intact, a controller using this would immediately publish the draft which would then automatically trigger search reindex and cache clearing.

@dfritschy

This comment has been minimized.

Copy link
Contributor

dfritschy commented Jun 15, 2017

@andrerom Well, you know me, I like to keep things as simple as possible. I think this is a rare use case not deserving a new API call given it can be done with existing API calls. So I would drop deleteTranslation() and rename purgeTranslations() to deleteTranslation() for the sake of consistency with the legacy API.

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jun 15, 2017

So I would drop deleteTranslation() a

Ok, makes sense.

consistency with the legacy API.

We don't keep consistency with legacy (legacy was not consistent in most regards)
We do try to care about consistently within the new api, and delete for the most part implies deleting entity in the rest of the api here, which is why we proposed purge. But other verbs might be more fitting for this. purge has some association with it also.

@dfritschy

This comment has been minimized.

Copy link
Contributor

dfritschy commented Jun 15, 2017

purge has some association with it also

so removeTranslations() might be best?

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jun 15, 2017

@dfritschy that can work also. @alongosz could you add your thoughts on all this when back on monday? And if you agree feel free to adapt the spec so we can move it forward to implementation.

@alongosz

This comment has been minimized.

Copy link
Member Author

alongosz commented Jun 19, 2017

I'm fine with removeTranslation() since it was like that in Legacy.

@dfritschy @andrerom

I think this is a rare use case not deserving a new API call given it can be done with existing API calls

I looked over content API once again and I don't really see a way to do it. Please keep in mind that if a field is required, current API won't allow setting its value to an empty one even if another translation exists.
Did I miss something?

I'm not exactly sure if the request for this feature initially was only about removing all translations, but I see following use case for removing the translation from a single version:

Imagine you have a multilingual site with default English content and an Article both in English and German. For some reason you want to remove German translation for now, only for this Article (e.g. there are mistakes in the translation and you don't have time to fix them).

One could argue that you could restore version w/o German translation, but what if restored English version is not up-to-date (some future version contains updates both to English and German)?

One could also argue that you could work around by setting English content in German version, however this approach might have unanticipated consequences and is wrong from the domain design POV.

But again, it's quite possible I'm missing something :)

@larseirik

This comment has been minimized.

Copy link

larseirik commented Jun 19, 2017

If there is a legal issue with the content object in one of the languages and you want to remove the given language version from the site this is not easily done today so I do think this should be supported.

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jun 19, 2017

@larseirik are you saying we should aim to keep deleteTranslation(VersionInfo $versionInfo, $languageCode); even if it might mean it will take longer to implement these features despite requests both here and from users else where where for the purgeTranslations(ContentInfo $contentInfo, $languageCode); only (now being discussed as removeTranslation())?

@alongosz regardless if it is possible or not with current api to remove from one version, I think we should split this out from this story and have as separate story in the backlog. Besides afaik publishing will merge the version draft given with what is in the current published one, hence current suggestion will run into issues as the translation you're trying to remove will then be merged back into the result and stay around (:

@alongosz

This comment has been minimized.

Copy link
Member Author

alongosz commented Jun 19, 2017

I think we should split this out from this story and have as separate story in the backlog.

I just thought the same thing. I'll make a story tomorrow :)

Besides afaik publishing will merge the version draft given with what is in the current published one, hence current suggestion will run into issues as the translation you're trying to remove will then be merged back into the result and stay around (:

Looking forward to the adventure that awaits me in the future then ;)

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jun 19, 2017

I just thought the same thing. I'll make a story tomorrow :)

thanks, and while at it update this PR so we can put the final result to a review and move forward with it ;)

@alongosz alongosz changed the title [Spec] EZP-27417: As a Developer I want API to allow removal of single translation w/o deleting the whole object [Spec] EZP-27417: As a Developer I want API to allow removal of the translation from all content object versions Jun 20, 2017

@alongosz alongosz force-pushed the alongosz:ezp-27417-translation-removal-spec branch from 5fc948d to 04b56df Jun 20, 2017

@alongosz

This comment has been minimized.

Copy link
Member Author

alongosz commented Jun 20, 2017

I've rewritten the spec taking into the account all review notes and removing parts related to the new extracted Story EZP-27508, so it's best if you look once more at the whole thing @andrerom @Nattfarinn (sorry! ... but at least it's shorter now :-) )

// cc @dfritschy @larseirik

@andrerom
Copy link
Member

andrerom left a comment

Looks good, but I'm missing some sort of invalid exception when language code is invalid for the given content.

@alongosz

This comment has been minimized.

Copy link
Member Author

alongosz commented Jun 20, 2017

Looks good, but I'm missing some sort of invalid exception when language code is invalid for the given content.

How about the same pair as in case of eZ\Publish\Core\Repository\LanguageService::loadLanguage?
@throws \eZ\Publish\API\Repository\Exceptions\InvalidArgumentException if languageCode argument is not string
@throws \eZ\Publish\API\Repository\Exceptions\NotFoundException if language could not be found

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jun 20, 2017

@throws \eZ\Publish\API\Repository\Exceptions\InvalidArgumentException if languageCode argument is not among the content translations

this one, not found is not valid here, we are not returning the language, and we type hint the content here so if it is not that is more of a runtime exception.

@alongosz

This comment has been minimized.

Copy link
Member Author

alongosz commented Jun 20, 2017

@throws \eZ\Publish\API\Repository\Exceptions\InvalidArgumentException if languageCode argument is not among the content translations

this one, not found is not valid here, we are not returning the language, and we type hint the content here so if it is not that is more of a runtime exception.

Added in e09da25 as a fixup, so squash merge required (or let me know, I'll squash it).

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jun 20, 2017

you can rebase / squash now :)

@alongosz alongosz force-pushed the alongosz:ezp-27417-translation-removal-spec branch from e09da25 to b5d2976 Jun 20, 2017

@alongosz

This comment has been minimized.

Copy link
Member Author

alongosz commented Jun 20, 2017

Done

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jun 20, 2017

Thanks @alongosz, merging. For implementation remember to as part of the PR move the spec here into some permanent home :)

@andrerom andrerom merged commit 79eb48d into ezsystems:master Jun 20, 2017

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ezrobot/phpcsfixer Code review by ezrobot
Details

@alongosz alongosz deleted the alongosz:ezp-27417-translation-removal-spec branch Jun 20, 2017

@SylvainGuittard

This comment has been minimized.

Copy link
Contributor

SylvainGuittard commented Jun 20, 2017

@larseirik sorry I am a little bit late on this

If there is a legal issue with the content object in one of the languages and you want to remove the given language version from the site this is not easily done today so I do think this should be supported.

Why not creating a new draft based on a previous version (with the modified language) and publish this draft? Easier and no need of a new feature in the API ;)

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jun 21, 2017

@SylvainGuittard :

Besides afaik publishing will merge the version draft given with what is in the current published one, hence current suggestion will run into issues as the translation you're trying to remove will then be merged back into the result and stay around (:

anyway, we move forward on the removal of translation from all versions, he can give input on the new story created for that, and we can re consider that for later. :shipit: :P

@bdunogier

This comment has been minimized.

Copy link
Member

bdunogier commented Jun 30, 2017

I'm pitching in a bit late, but I was expecting that both options would be possible, something like:

/**
 * Purges a Language from all versions of a Content.
 */
public function purgeLanguage(ContentInfo $contentInfo, Language $language);

/**
 * Deletes a Language from a Draft.
 */
public function purgeLanguage(ContentInfo $contentInfo, Language $language);

The part that is a bit odd is that you would I think only purge a language if you want to remove it. But in that case, you would probably expect that this is done when you remove the language.

The other feels much more natural, since it doesn't rewrite the archived versions of a content. You could imagine a scenario where a translation is added, but turns out to be incorrect. It feels perfectly valid to publish a new version that doesn't have this translation anymore, work on it again, and republish a new one where it got fixed.

@alongosz

This comment has been minimized.

Copy link
Member Author

alongosz commented Jun 30, 2017

The part that is a bit odd is that you would I think only purge a language if you want to remove it.

I initially proposed purge... thinking exactly about such use case. Finally we decided on removeTranslations because it was called that in Legacy as I was told.
I already have the implementation for this in #2031 pending approval.

The other feels much more natural

I agree. This will be implemented separately for EZP-27508.

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jul 5, 2017

The other feels much more natural, since it doesn't rewrite the archived versions of a content. You could imagine a scenario where a translation is added, but turns out to be incorrect. It feels perfectly valid to publish a new version that doesn't have this translation anymore, work on it again, and republish a new one where it got fixed.

That is exactly why we initially added public function deleteTranslation(VersionInfo $versionInfo, $languageCode);, but:

  1. neither @dfritschy or PM saw the need for it
  2. It probably won't work with current publishing logic, as soon as you publish it will merge in languages from previous published version

So we postponed this bit until later.

@alongosz alongosz restored the alongosz:ezp-27417-translation-removal-spec branch Sep 8, 2017

@alongosz alongosz deleted the alongosz:ezp-27417-translation-removal-spec branch Sep 8, 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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.