Comments

Anushka-mp’s picture

Issue summary: View changes
Anushka-mp’s picture

Issue summary: View changes
sasanikolic’s picture

Status: Active » Needs review
StatusFileSize
new4.64 KB
new85.28 KB

Paragraphs work now.

Possibly have to fix those empty divisions under the title.

Status: Needs review » Needs work

The last submitted patch, 3: support_paragraphs-2462263-3.patch, failed testing.

sasanikolic’s picture

Forgot a debug in the patch, will remove it when I get a review and when we have a solution for the test fail.

berdir’s picture

Issue tags: +Needs tests
  1. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
    @@ -16,6 +18,7 @@ use Drupal\tmgmt\JobItemInterface;
     use Drupal\Core\Render\Element;
    +use Symfony\Component\Validator\Constraints\True;
    

    Wrong use.

  2. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
    @@ -63,21 +66,34 @@ class ContentEntitySource extends SourcePluginBase {
    +  /**
    +   * Function that gets all the levels (for paragraphs).
    +   *
    +   * @param $translatable_fields
    +   * @param $translation
    +   */
    +  public function getLevels(ContentEntityInterface $translation) {
    

    That method name doesn't make sense for me. It doesn't get levels (I'm not sure what that means ;))

    What it does is extract translatable data, so I'd go with extractTranslatableData(), just like in the config source.

    Also, documentation needs work, no need to need to mention paragraphs here, instead it should be documented in the code below what we're doing and why.

  3. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
    @@ -86,11 +102,15 @@ class ContentEntitySource extends SourcePluginBase {
    +          debug(get_class($property));
    +          if ($property instanceof EntityReference) {
    +            // $property->getValue();
    +            if($property->getValue() instanceof ContentEntityInterface) {
    +              $data[$key][$index][$property_key] = $this->getLevels($property->getValue());
    +            }
    +          }
    

    The whole block can be removed again.

  4. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
    @@ -119,6 +139,27 @@ class ContentEntitySource extends SourcePluginBase {
    +      $field = $translation->get($key);
    +      $data[$key]['#label'] = $field_definition->getLabel();
    +      foreach ($field as $index => $field_item) {
    +        $data[$key][$index]['#label'] = t('Delta #@delta', array('@delta' => $index));
    +        $format = NULL;
    +
    +        /* @var FieldItemInterface $field_item */
    +        foreach ($field_item->getProperties(TRUE) as $property_key => $property) {
    +          if ($property instanceof EntityReference) {
    +            if ($property->getValue() instanceof ContentEntityInterface) {
    

    The reason you get empty divs is because you define labels for elements that then don't have any data inside them. You need to make sure that you only set this when you actually have data, by setting it inside the if(), for example.

    Also, the next step is to add a setting to tmgmt_content that allows to control to which entity types this should be done. There we can also describe this behavior.

Needs tests, which will obviously only work after core and paragraphs is patched. But we can write them already.

sasanikolic’s picture

Status: Needs work » Needs review
StatusFileSize
new11.62 KB
new11.34 KB

Done the embedded references and a test for the checked reference fields.

berdir’s picture

Status: Needs review » Needs work

Cool, this is starting to come together, still quite a bit of work needed...

  1. diff --git a/sources/content/config/install/tmgmt_content.settings.yml b/sources/content/config/install/tmgmt_content.settings.yml
    new file mode 100644
    index 0000000..e69de29
    

    the new file should have an empty definition for the new key, embedded_fields: { }

  2. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
    @@ -63,35 +65,42 @@ class ContentEntitySource extends SourcePluginBase {
         if (!$entity->hasTranslation($job_item->getJob()->getSourceLangcode())) {
           throw new TMGMTException(t('The entity %id with translation %lang does not exist.', array('%id' => $entity->id(), '%lang' => $job_item->getJob()->getSourceLangcode())));
         }
     
    +    $translation = $entity->getTranslation($job_item->getJob()->getSourceLangcode());
    

    This part here is interesting. We should do the same for embedded references. And if they don't have the source language, then probably skip them.

  3. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
    @@ -63,35 +65,42 @@ class ContentEntitySource extends SourcePluginBase {
    +  /**
    +   * Function that gets all the translatable data.
    +   *
    

    I can see that it is a function :)

    Always start with a verb in third person when describing what a function does. In this case...

    "Extracts translatable data from an entity."

  4. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
    @@ -63,35 +65,42 @@ class ContentEntitySource extends SourcePluginBase {
    +   * @param \Drupal\Core\TypedData\TranslatableInterface $translation
    +   */
    

    Wondering if we should rename this to $entity.

    Also, description missing, and @return as well.

  5. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
    @@ -119,6 +135,31 @@ class ContentEntitySource extends SourcePluginBase {
    +    $untranslatable_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);
    +    });
    +    foreach ($untranslatable_fields as $key => $field_definition) {
    

    Now, we need to use the new setting here, because right now, we don't use it anywhere.

    Instead of the $exclude_field_types here, what we want to check is that the field name is in the list of enabled fields for that entity type.

    Also note that we only consider untranslatable fields here. (I will get back to this).

    Then, we need tests for the actual logic here.

    I would suggest a new method in ContentEntityUnitTest. You can start with one of the existing methods, for example for EntityTest. And then add a reference field (this needs entity_reference.module) that points to entities of the same type (for simplicity). Enable that field in your settings, then create two entities: A and B that references A. Then request data for it, and save it back.

    Saving back will then fail, because it is not implemented yet :)

  6. +++ b/sources/content/src/Tests/ContentEntitySourceUiTest.php
    @@ -341,4 +341,27 @@ class ContentEntitySourceUiTest extends EntityTestBase {
    +    $this->drupalGet('admin/config/regional/tmgmt_settings');
    +
    +    $checked_reference_fields = array(
    +      'embedded_fields[node][type]' => TRUE,
    +      'embedded_fields[node][uid]' => TRUE,
    +    );
    

    You should add a few asserts here about fields that should *not* be present, because we filter them out.

  7. +++ b/sources/content/tmgmt_content.module
    @@ -54,3 +56,86 @@ function tmgmt_content_tmgmt_source_suggestions(array $items, JobInterface $job)
    +    if ($content_translation_manager->isEnabled($entity_type->id())) {
    +      $storage_definitions = \Drupal::entityManager()->getFieldStorageDefinitions($entity_type->id());
    +
    +      foreach ($storage_definitions as $storage_definition) {
    

    Coming back to the isTranslatable() check from above. We want to check that here as well. Which means that most fields will go away and we will need to explicitly set one up for testing. Just like in the unit test.

    The description above should then also explain that this will only work for fields that are not translatable.

  8. +++ b/sources/content/tmgmt_content.module
    @@ -54,3 +56,86 @@ function tmgmt_content_tmgmt_source_suggestions(array $items, JobInterface $job)
    +/**
    + * Submit function.
    + *
    + * @param array $form
    + * @param \Drupal\Core\Form\FormStateInterface $form_state
    + */
    +function tmgmt_content_settings_submit(array &$form, FormStateInterface $form_state) {
    

    @param for form callbacks are not needed, just remove it. However, the description should somehow reference about which form it is.

  9. +++ b/sources/content/tmgmt_content.module
    @@ -54,3 +56,86 @@ function tmgmt_content_tmgmt_source_suggestions(array $items, JobInterface $job)
    +  foreach ($form_state->getValue('embedded_fields') as $key=>$fields) {
    +    foreach (array_filter($fields) as $id=>$label) {
    +      $embedded_fields[$key][$id] = TRUE;
    +    }
    

    check coding standard here.

sasanikolic’s picture

Part of the third patch. Needs to be finished.

berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new27.46 KB
new24.45 KB

Ok, lots of additional checks, improved tests, implemented saving translations. Could use some code reviews and testing, I've only run the tests and the code is pretty complicated in some places, a bit of refactoring wouldn't hurt.

miro_dietiker’s picture

Yay, this looks awesome. Some notes.

  1. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
    @@ -63,35 +65,46 @@ class ContentEntitySource extends SourcePluginBase {
    +   * Extracts translatable data from an entity.
    
    @@ -212,5 +236,45 @@ class ContentEntitySource extends SourcePluginBase {
    +   * Saves translation data in an entity translation.
    

    Preferrably add a comment about the embedded fields recursion of the call and the intermediate data structure.

  2. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
    @@ -63,35 +65,46 @@ class ContentEntitySource extends SourcePluginBase {
         $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);
    +        return $field_definition->isTranslatable() && !in_array($field_definition->getType(), $exclude_field_types);
    

    Hm, altered indentation looks wrong to me.

  3. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
    @@ -63,35 +65,46 @@ class ContentEntitySource extends SourcePluginBase {
               $property_definition = $property->getDataDefinition();
    -          if (($property_definition->isComputed())) {
    -            continue;
    -          }
    

    Why now processing computed values?

berdir’s picture

2. Keep in mind that it is inside an anonymous function, it's phpstorm that changed that.
3. because getProperties() doesn't return them in the first place, that was a left-over of when it did do that. Not directly related to the other changes.

berdir’s picture

Wasn't aware that entity_reference_revisions has a separate data type (which is called entity_revision_reference because reasons), this should work with both now.

sasanikolic’s picture

Saving was broken. This should fix it.

Status: Needs review » Needs work

The last submitted patch, 14: support_paragraphs-2462263-14.patch, failed testing.

miro_dietiker’s picture

Corrected...

miro_dietiker’s picture

Status: Needs work » Fixed

Committed. More fun in followups.

Status: Fixed » Closed (fixed)

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