Problem/Motivation

Same as #2355909: language.settings config is not scalable, needs that for third party settings storage, so it has a place to store our settings.
Postponed on #2355909: language.settings config is not scalable

Proposed resolution

Use third party settings on language settings to store content translation settings.

Remaining tasks

Do it.

User interface changes

None.

API changes

content_translation_set_config(), content_translation_get_config(), content_translation_get_config_key() would be replaced/removed. May be kept for BC.

Comments

Gábor Hojtsy’s picture

Issue tags: +API change, +D8 upgrade path
Gábor Hojtsy’s picture

tstoeckler’s picture

Component: config_translation.module » content_translation.module

:-)

YesCT’s picture

Issue summary: View changes
YesCT’s picture

Issue summary: View changes
catch’s picture

Status: Postponed » Active
penyaskito’s picture

Status: Active » Needs review
FileSize
2.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Use ContentLanguageSettings third party settings storage. Kept the functions for now, not sure if we want to leave them, deprecate them or remove them.

penyaskito’s picture

PS: IMHO we should remove them before it is too late.

Status: Needs review » Needs work

The last submitted patch, 7: 2363155-content-translations-7.patch, failed testing.

andypost’s picture

+1 to deprecate procedural wrappers
PS: There's actually a few failures

penyaskito’s picture

Status: Needs work » Needs review
FileSize
3.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,108 pass(es), 1 fail(s), and 1 exception(s). View
754 bytes

I didn't know that you need to provide a schema for third_party settings, but makes lots of sense.

penyaskito’s picture

Found this @todo:

/**
 * Determines whether the given entity type is translatable.
 *
 * @param string $entity_type
 *   The type of the entity.
 * @param string $bundle
 *   (optional) The bundle of the entity. If no bundle is provided, all the
 *   available bundles are checked.
 *
 * @returns
 *   TRUE if the specified bundle is translatable. If no bundle is provided
 *   returns TRUE if at least one of the entity bundles is translatable.
 *
 * @todo Move to \Drupal\content_translation\ContentTranslationManager.
 */
function content_translation_enabled($entity_type, $bundle = NULL) {

Proposal:

Remove:

  • content_translation_enabled()
  • content_translation_set_config()
  • content_translation_get_config()
  • content_translation_get_config_key()

Add:

  • ContentTranslationManager::enable($entity_type, $bundle) : void
  • ContentTranslationManager::disable($entity_type, $bundle) : void
  • ContentTranslationManager::isEnabled($entity_type, $bundle) : bool

Status: Needs review » Needs work

The last submitted patch, 11: 2363155-content-translations-11.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
4.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,116 pass(es). View
880 bytes

That was not my fault... We are supposed to be using existing entities.

penyaskito’s picture

FileSize
21.56 KB
23.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,136 pass(es). View

Removing:

  • content_translation_enabled()
  • content_translation_set_config()
  • content_translation_get_config()
  • content_translation_get_config_key()

Adding:

  • ContentTranslationManager::enable($entity_type, $bundle) : void
  • ContentTranslationManager::disable($entity_type, $bundle) : void
  • ContentTranslationManager::setEnabled($entity_type_id, $bundle, $value) : void
  • ContentTranslationManager::isEnabled($entity_type, $bundle) : bool
Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +sprint

Looks generally good except:

  1. +++ b/core/modules/content_translation/config/schema/content_translation.schema.yml
    @@ -10,3 +10,10 @@ field_config.third_party.content_translation:
               label: 'Field column for which to synchronize translations'
    +content_settings.third_party.content_translation:
    

    Should have an empty line before this one.

    Also the naming of this schema element is *confusing*. Should it be language.content_settings.*? I know this should have been said in #2355909: language.settings config is not scalable but not too late here either :)

    Making top level keys like that is confusing because there may be a content_settings module in contrib which would have this namespace. Let's keep core schema elements in the core namespace.

  2. +++ b/core/modules/content_translation/src/ContentTranslationManager.php
    @@ -52,4 +53,47 @@ public function getSupportedEntityTypes() {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function setEnabled($entity_type_id, $bundle, $value) {
    +    $config = ContentLanguageSettings::loadByEntityTypeBundle($entity_type_id, $bundle);
    +    $config->setThirdPartySetting('content_translation', 'enabled', $value)->save();
    +  }
    

    I think its confusing to have enable/disable and setEnabled all, sounds confusing.

  3. +++ b/core/modules/content_translation/src/ContentTranslationPermissions.php
    @@ -27,20 +27,33 @@ class ContentTranslationPermissions implements ContainerInjectionInterface {
    +s   */
    

    Extra s :)

plach’s picture

Nice :)

  1. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -74,7 +74,7 @@ function _content_translation_form_language_content_settings_form_alter(array &$
    +    $default[$entity_type_id] = $enabled || \Drupal::service('content_translation.manager')->isEnabled($entity_type_id) ? $entity_type_id : FALSE;
    
    @@ -118,7 +118,7 @@ function _content_translation_form_language_content_settings_form_alter(array &$
    +            '#default_value' => \Drupal::service('content_translation.manager')->isEnabled($entity_type_id, $bundle),
    
    @@ -313,7 +313,7 @@ function content_translation_form_language_content_settings_submit(array $form,
    +        \Drupal::service('content_translation.manager')->setEnabled($entity_type_id, $bundle, $bundle_settings['translatable']);
    

    Can we retrieve the content manager just once and save it in a $manager variable? That would improve readability.

  2. +++ b/core/modules/content_translation/src/ContentTranslationManagerInterface.php
    @@ -31,4 +31,52 @@ public function getSupportedEntityTypes();
    +  public function enable($entity_type_id, $bundle);
    ...
    +  public function disable($entity_type_id, $bundle);
    ...
    +  public function setEnabled($entity_type_id, $bundle, $value);
    

    I agree with Gabor, having the three methods is confusing. I'd keep only the setter.

yched’s picture

[edit : never mind, @Gabor was quicker than me]

penyaskito’s picture

Status: Needs work » Needs review
FileSize
11.13 KB
23.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,106 pass(es). View

Adressing #16, #17, #18.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Sorry I did not find this one before:

+++ b/core/modules/content_translation/src/ContentTranslationManager.php
@@ -52,4 +53,32 @@ public function getSupportedEntityTypes() {
+        if ($config->getThirdPartySetting('content_translation', 'enabled', false)) {

false should be FALSE in PHP in Drupal's code style.

Looks good otherwise.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
887 bytes
23.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,108 pass(es). View

Fixed.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Amazing, looks all good to me :)

plach’s picture

Awesome work!

RTBC +1

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This looks fantastic. One very small thing.

+++ b/core/modules/content_translation/src/ContentTranslationManager.php
@@ -52,4 +53,32 @@ public function getSupportedEntityTypes() {
+    $config = ContentLanguageSettings::loadByEntityTypeBundle($entity_type_id, $bundle);
...
+        $config = ContentLanguageSettings::loadByEntityTypeBundle($entity_type_id, $bundle);

Let's not use the static functions here. We should add a protected method to load everything from the already injected entity manager.

  protected function loadContentLanguageSettings($entity_type_id, $bundle) {
    if ($entity_type_id == NULL || $bundle == NULL) {
      return NULL;
    }
    $config = $this->entityManager->getStorage('language_content_settings')->load($entity_type_id . '.' . $bundle);
    if ($config == NULL) {
      $config = $this->entityManager->getStorage('language_content_settings')->create(['target_entity_type_id' => $entity_type_id, 'target_bundle' => $bundle]);
    }
    return $config;
  }
penyaskito’s picture

Status: Needs work » Needs review
FileSize
2.24 KB
24.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,063 pass(es). View

Fixed #24.

Gábor Hojtsy’s picture

Hum, I am not sure this is an improvement? I mean loadByEntityBundle nicely abstracted this fallback / decision logic and now it is here again :/

alexpott’s picture

@Gábor Hojtsy those static loaders should not be used in injected classes - they obscure dependencies and prevent simple unit testing.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/content_translation/src/ContentTranslationManager.php
@@ -52,4 +53,55 @@ public function getSupportedEntityTypes() {
+   * @return $this
+   *   The content language config entity if one exists. Otherwise, returns
+   *   default values.

This does not return $this - it returns the config entity.

andypost’s picture

The related issue should be in-sync with this one

penyaskito’s picture

Status: Needs work » Needs review
FileSize
649 bytes
24.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,051 pass(es). View

Good catch, fixed the return type doc.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Nice, thanks! I think this resolves all concerns. Thanks @alexpott for pointing out that the local nice-ness would actually be problematic for injectability. Make sense.

alexpott’s picture

Issue tags: +Needs change record

We should have a d8 to d8 CR record for this to help existing multilingual sites. That CR should reference https://www.drupal.org/node/2382481

Gábor Hojtsy’s picture

Issue tags: -Needs change record
plach’s picture

Looks great

plach’s picture

Issue tags: +Ghent DA sprint
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4efd487 and pushed to 8.0.x. Thanks!

  • alexpott committed 4efd487 on 8.0.x
    Issue #2363155 by penyaskito: content_translation.settings config is not...
Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks again @penyaskito!

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks again @penyaskito!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.