Add ability to exclude translatable fields from being handled by TMGMT.

Sounds like a useful feature. I think it could work in a similar way as the embedded fields setting that we already have. The UI might not scale if we show dozens of fields in a checkbox though, might need some kind of custom autocomplete instead, or expose it on the field settings page.

Seems to be a useful feature in plan of UI to add it there.

Per @bonus originally stated method "extractTranslatableData" I started to dig more into fields exclusion and noticed the following condition inside of this method:

$exclude_field_types = ['language'];
$translatable_fields = array_filter($field_definitions, function (FieldDefinitionInterface $field_definition) use ($exclude_field_types) {
        return $field_definition->isTranslatable() && !in_array($field_definition->getType(), $exclude_field_types);
    });

Basically, getType() returns the entity plugin type, ex: text, while we need to target on getName() which relates to field machine names definition and it makes sense to exclude the fields based on their machine_names rather than Plugin Types.

So, the code should look like:

$exclude_field_types = ['language'];
$translatable_fields = array_filter($field_definitions, function (FieldDefinitionInterface $field_definition) use ($exclude_field_types) {
        return $field_definition->isTranslatable() && !in_array($field_definition->getName(), $exclude_field_types);
    });

So this will lead to a more granular approach or we can combine both.

No, you don't want to replace that, we still want to exclude some field types. But extend that logic with a field name based exclusion list, either as a third party settings flag (would only work for configurable field definitions) or a global list, similar to the embedded field list.. sure, why not.

@Berdir, what do you think about the possibility to give access to $translatable_fields list through some alter hook? So that other modules could filter/alter this list according to their own logic?

Sure, that's fine with me. Still think a setting would be useful as well, but a hook makes sense either way.

Ok, so to start something with:

Thanks.

1. +++ b/sources/content/tmgmt_content.api.php
   @@ -23,5 +24,18 @@ function hook_tmgmt_content_list_query_alter(QueryInterface $query) {
   + * Allows to exclude some fields from translation with TMGMT
   

   Missing . at the end.

2. +++ b/sources/content/tmgmt_content.api.php
   @@ -23,5 +24,18 @@ function hook_tmgmt_content_list_query_alter(QueryInterface $query) {
   + * @see www.drupal.org/node/2823515
   

   we usually don't put @see on issues in the docs unless they provide important information which I think is not the case here.

   Also, @see would have to be after @param, after an empty line.

3. +++ b/sources/content/tmgmt_content.api.php
   @@ -23,5 +24,18 @@ function hook_tmgmt_content_list_query_alter(QueryInterface $query) {
   + * @param ContentEntityInterface $entity
   + * @param $translatable_fields
   

   type is required for all arguments and must be fully qualified

4. +++ b/sources/content/tmgmt_content.api.php
   @@ -23,5 +24,18 @@ function hook_tmgmt_content_list_query_alter(QueryInterface $query) {
   +function hook_tmgmt_translatable_fields_alter(ContentEntityInterface $entity, &$translatable_fields) {
   

   arguments to hook-documenation-functions should also use the full namespace so they are easier to copy into a module.

Thanks for the quick review! Fixed suggestions above.

HI Guys,
I will submit smth for review as well shortly. Added some UI/UX approach for the user per FieldConfig.
Will post shortly.

Attaching patch which includes the UI approach through ThirdPartySettings.

Thanks. Conceptually on the right track but quite a few things to clean up and improve.

Also, the two patches should be combined into one. And we'll need tests, at least for the third party settings part, I can live with not having tests for the hook.

1. +++ b/config/schema/tmgmt.schema.yml
   @@ -76,3 +76,11 @@ views.filter.tmgmt_job_type_filter:
              label: 'Hide this filter if there are no continuous jobs.'
   +
   +node.type.*.third_party.tmgmt_excluded_fields:
   +  type: mapping
   +  label: 'Excluded Fields'
   +  mapping:
   

   this seems wrong, the third party setting is on the field config entity, not node types.

2. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
   @@ -121,6 +123,29 @@ class ContentEntitySource extends SourcePluginBase implements SourcePreviewInter
   +      if(method_exists($field_definition, 'getThirdPartySetting')){
   

   You should check for the interface with instanceof: "if ($field_definition instanceof ThirdPartySettingsInterface) {"

   Also check coding standards, like in my example above with spaces.

3. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
   @@ -136,10 +161,15 @@ class ContentEntitySource extends SourcePluginBase implements SourcePreviewInter
    
   +    //Exclude field names from translation.
   +    $translatable_fields = $this->excludeFieldNames($field_definitions, $translatable_fields);
   +
   

   I don't think we need a separate method for this (which would need proper documentation).

   Can't we do this inside the existing array_filter() callback?

   What we could do is make the anonymous function for array_filter() an actual method on this object, but this shouldn't get that complicated.

   For example, we could refactor it to separate the checks into 3 different ifs. if not transable, return false. if excluded field type, return false. if implements that interface and the checkbox is enabled, return false. end with return true.

4. +++ b/tmgmt.module
   @@ -1280,3 +1280,44 @@ function tmgmt_help($route_name, RouteMatchInterface $route_match) {
   +/**
   + * Implements hook_FROM_ID_alter().
   + * @param                                      $form
   + * @param \Drupal\Core\Form\FormStateInterface $form_state
   + */
   +function tmgmt_form_field_config_edit_form_alter(&$form, FormStateInterface $form_state) {
   +
   +
   

   hook implementations don't need to document their params.

   This should live in tmgmt_entity.module, not in tmgmt.module itself, that doesn't know anything about fields.

5. +++ b/tmgmt.module
   @@ -1280,3 +1280,44 @@ function tmgmt_help($route_name, RouteMatchInterface $route_match) {
   +  $form['tmgmt_exclude_field'] = array(
   +    '#type' => 'checkbox',
   +    '#title' => t('Exclude from TMGMT translations'),
   +    '#weight' => -8,
   +    '#default_value' => $field_config->getThirdPartySetting('tmgmt_excluded_fields_excluded', 'excluded', 0),
   +  );
   

   I think we usually don't use TMGMT in UI text, not sure about adding this here.

   The first argument to getThirdPartySetting() and all related methods is the module name.

6. +++ b/tmgmt.module
   @@ -1280,3 +1280,44 @@ function tmgmt_help($route_name, RouteMatchInterface $route_match) {
   + * Implements hook_form_id_alter().
   + * @param                                      $entity_type
   + * @param                                      $type
   + * @param                                      $form
   + * @param \Drupal\Core\Form\FormStateInterface $form_state
   + */
   +function tmgmt_form_field_config_edit_exclude_settings($entity_type, $type, &$form, FormStateInterface $form_state){
   

   I'd also leave out params here, also this is not a hook but an entity builder callback. And I'd just entity_builder in the function name.

   And $type should be $entity

Thanks @berdir for your prompt review and feedback. I fall in agreement with all stated above and will work on refactoring based on comments and roll-out the updated patch including them.

Kind regards,
S.

@berdir, could you please review the re-created patch bellow.
Meanwhile, will work on tests for it.
Thanks!

When making changes to an existing patch, please provide an interdiff: https://www.drupal.org/documentation/git/interdiff

How is it going with the tests?

Hi Berdir,
thanks for following up - I was a bit off with other things, but now I'm back.

I took a look into CrudTest file, but there is a think which creates questions.
While I use thirdpartysettings for setting up excluded from translation property on entity in real world versus the for example "testAddingTranslatedData", where the test data is generated and not taken from real world (real fields/entities), which is fine and should not intersect with each other...

I'm not sure how to fake the field with excluded property for my test case.
Would appreciate if you can drop a suggestion?

Thanks
S.

Sorry, I don't understand your question.

The entities and fields within the tests are 100% real, just like on a real website. You can edit them and add third party settings through the API, just like you do with the form.

Hi @Berdir, sorry for a delay on this one. But I have some issues with tests:

1. I ended up creating a custom content type in tmgmt_tests module, so I could work with real fields. Also, while we use clean profile to run tests this was required to be done so.

2. When I run the tests, I'm under impression that content type does not get created in test environment, while if I try it locally by enabling tmgmt_test module it appears.

I'm testing content types through following method:
$node_types = \Drupal\node\Entity\NodeType::loadMultiple();
dd($node_types, 'types');

while running the test and it returns empty in test env, while on local it is fine.

Would appreciate any insights if you could help to better understand this test env behavior.
Thanks,
S.

1. No, it really shouldn't be needed. Again, the fields that we create in the test are 100% real, they just have dynamic names and are for dynamically named node types.

For example in \Drupal\Tests\tmgmt_content\Kernel\ContentEntitySourceUnitTest::setUp, all you need to do is something like this:

    $field_storage = FieldStorageConfig::create(array(
      'field_name' => 'ignored_field',
      'entity_type' => $this->entityTypeId,
      'type' => 'image',
      'cardinality' => FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED,
      'translatable' => TRUE,
    ));
    $field_storage->save();
    FieldConfig::create(array(
      'entity_type' => $this->entityTypeId,
      'field_storage' => $field_storage,
      'bundle' => $this->entityTypeId,
      'label' => $this->image_label = $this->randomMachineName(),
    ))
      ->setThirdPartySetting('tmgmt_content', 'excluded', 1)
      ->save();

I copied this from the existing field definition, I only changed the name, field type and added the third party settings call. Now you have a field that will be ignored.

You just need to set a value for it a bit below with $translation->ignored_field->value = 'This must not be translated';

And then you can assert later below that ignored_field is, unlike field_test_text and image_test *not* in $data and you're done.

Also, just noticed one more thing in the patch.. true/false must be upper case.

+1 for this feature.

Attaching updated patch which includes test stuff and some minor tweaks. Hopefully it can help move it along.

Thanks, looks good, just coding standards issues mostly to clean up.

1. +++ b/config/schema/tmgmt.schema.yml
   @@ -82,3 +82,4 @@ views.filter.tmgmt_job_type_filter:
              label: 'Hide this filter if there are no continuous jobs.'
   +          ¶
   \ No newline at end of file
   

   some trailing spaces here.

2. +++ b/sources/content/config/schema/tmgmt_content.schema.yml
   @@ -8,3 +8,11 @@ tmgmt_content.settings:
   +      type: boolean
   +      label: 'Exclude field from translation'          ¶
   diff --git a/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
   

   Same here. Noticed this in the redirect patch that you worked on as well, consider configuring your editor/IDE to remove those.

3. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
   @@ -136,10 +140,32 @@ class ContentEntitySource extends SourcePluginBase implements SourcePreviewInter
   +      if (($field_definition instanceof ThirdPartySettingsInterface)) {
   +        $is_excluded = $field_definition->getThirdPartySetting('tmgmt_content', 'excluded', 0);
   +        if (!empty($is_excluded)) {
   +          return FALSE;
   

   why 0 as default value and not FALSE? And why not just if ($is_excluded), the variable is always defined and either true or false already.

4. +++ b/sources/content/tests/src/Kernel/ContentEntitySourceUnitTest.php
   @@ -101,6 +101,23 @@ class ContentEntitySourceUnitTest extends EntityKernelTestBase {
   +    ))->setThirdPartySetting('tmgmt_content', 'excluded', 1)
   

   also here, use TRUE

5. +++ b/sources/content/tmgmt_content.api.php
   @@ -18,10 +16,22 @@ use Drupal\Core\Entity\Query\QueryInterface;
   + * @param Drupal\Core\Entity\ContentEntityInterface $entity
   + * @param Drupal\Core\Field\BaseFieldDefinition[]   $translatable_fields
   + */
   

   leading \ missing for the class names, fields are FieldDefinitionInterface, not just base fields. And desription is also needed.

6. +++ b/sources/content/tmgmt_content.module
   @@ -319,3 +319,38 @@ function tmgmt_content_entity_access(\Drupal\Core\Entity\EntityInterface $entity
   + * Entity builder callback.
   + * @param string $entity_type
   + * @param \Drupal\field\Entity\FieldConfig $entity
   + * @param array $form
   + * @param \Drupal\Core\Form\FormStateInterface $form_state
   + */
   +function tmgmt_content_entity_builder($entity_type, $entity, &$form, FormStateInterface $form_state) {
   

   since the builder is a callback, @params are IMHO not needed, they're also not correctly formatted.

7. +++ b/sources/content/tmgmt_content.module
   @@ -319,3 +319,38 @@ function tmgmt_content_entity_access(\Drupal\Core\Entity\EntityInterface $entity
   +  if (!empty($exclude_from_translation)) {
   +    $entity->setThirdPartySetting('tmgmt_content', 'excluded', 1);
   +  }
   +  else {
   +    $entity->setThirdPartySetting('tmgmt_content', 'excluded', 0);
   +  }
   

   you can actually write this in 1-2 lines again, setThirdPartySetting($exclude_from_translation)

Thanks for reviewing, @berdir.

Attached patch addresses all of the above. Tweaked my editor settings and also removed some brackets and use statements which were added in the original patch in #20 but whose classes I couldn't see being referenced anywhere.

FYI, when I ran, ../../vendor/bin/phpunit --group tmgmt, I got an error about missing configuration schema, but I haven't addressed that.

1) Drupal\Tests\tmgmt_config\Kernel\ConfigSourceUnitTest::testView
InvalidArgumentException: The configuration property display.default.display_options.filters.job_type.group_info.group_items.1.value.value doesn't exist.

That config error sounds like you were on 8.2, I think the tests only work with 8.3+.

Thanks, looks good to me now, committed.