Problem/Motivation

Given an entity is in a job as item and the entity is deleted.
When the job item then is accepted, there is a fatal error.

Proposed resolution

Check existence of the entity.
Check other sources if they also properly check the existing source item situation.
Also add error messages if inexisting. The source should addMessage if it can not do its job.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#16 tmgmt_2543294_delete_16.interdiff.txt4.16 KBmiro_dietiker
#16 tmgmt_2543294_delete_16.patch7.85 KBmiro_dietiker
#14 tmgmt_2543294_delete_14.patch7.83 KBmiro_dietiker
#14 tmgmt_2543294_delete_14.interdiff.txt4.15 KBmiro_dietiker
#9 2543294-complete-9.patch5.67 KBgiancarlosotelo
#9 interdiff-2543294-4-9.txt3.05 KBgiancarlosotelo
#4 2543294-complete-4.patch5.59 KBgiancarlosotelo
#4 interdiff-3-4.txt789 bytesgiancarlosotelo
#3 2543294-complete-3.patch6.17 KBgiancarlosotelo
#3 2543294-3_TESTONLY.patch2.37 KBgiancarlosotelo

Comments

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Note this affects
JobInterface::acceptTranslation
SourcePluginInterface::saveTranslation
LocaleSource::updateTranslation

giancarlosotelo’s picture

giancarlosotelo commented
Assigned: Unassigned » giancarlosotelo
giancarlosotelo’s picture

giancarlosotelo commented
Status: Active » Needs review
StatusFileSize
new 2543294-3_TESTONLY.patch2.37 KB
new 2543294-complete-3.patch6.17 KB

Uploading a testonly patch.
I also checked a fatal error when a Config Entity is deleted. Is quite different from Content Entity because of the ConfigMapper that throws an exception so I added a try/catch to show a message and avoid the fatal error.
About LocaleSource I checked and seems that no changes are needed.

Tests are going to fail because of recent core changes, but some feedback is appreciated.

giancarlosotelo’s picture

giancarlosotelo commented
StatusFileSize
new interdiff-3-4.txt789 bytes
new 2543294-complete-4.patch5.59 KB

Something was wrong

The last submitted patch, 3: 2543294-3_TESTONLY.patch, failed testing.

The last submitted patch, 3: 2543294-complete-3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: 2543294-complete-4.patch, failed testing.

juanse254’s picture

juanse254 commented 
  1. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
@@ -170,11 +170,16 @@ class ContentEntitySource extends SourcePluginBase {
+      $job_item->addMessage('The entity %id of type %type does not exist, the job can not be completed.', array('%id' => $job_item->getItemId(), '%type' => $job_item->getItemType()), 'error');

    Can we split this to make it more readable? 

  2. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
@@ -170,11 +170,16 @@ class ContentEntitySource extends SourcePluginBase {

    Remove empty line? 

  3. +++ b/sources/content/src/Tests/ContentEntitySourceUiTest.php
@@ -169,6 +169,22 @@ class ContentEntitySourceUiTest extends EntityTestBase {
+    // Test the translation of a deleted entity.

    I guess here we are trying to delete/test a node without an entity, not testing a translation of a deleted entity. 

  4. +++ b/sources/content/src/Tests/ContentEntitySourceUiTest.php
@@ -169,6 +169,22 @@ class ContentEntitySourceUiTest extends EntityTestBase {
+    $this->drupalGet('node/' . $deleted_node->id());
+    $this->clickLink('Translate');

    Guess you merge this and save a line. ex: $val . '/translatable' 

  5. +++ b/sources/tmgmt_config/src/Tests/ConfigSourceUiTest.php
@@ -238,6 +239,16 @@ class ConfigSourceUiTest extends EntityTestBase {
+    $this->drupalPostForm(NULL, array(), t('Save as completed'));

    The proper way would be [] .

Besides that the tests are failing due to #2553801: SafeMarkup Change, tmgmt broken so as soon as that gets fixed the tests are going to pass.

giancarlosotelo’s picture

giancarlosotelo commented
StatusFileSize
new interdiff-2543294-4-9.txt3.05 KB
new 2543294-complete-9.patch5.67 KB

1 . Splitted.
2. I think there is no empty line.
3. A better description.
4. Merged.
5. In the whole test array() is used so I think it doesn't matter.

Still waiting for the fix in the head.

giancarlosotelo’s picture

giancarlosotelo commented
Status: Needs work » Needs review

giancarlosotelo queued 3: 2543294-3_TESTONLY.patch for re-testing.

The last submitted patch, 3: 2543294-3_TESTONLY.patch, failed testing.

juanse254’s picture

juanse254 commented
Status: Needs review » Reviewed & tested by the community

Looking good now.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new tmgmt_2543294_delete_14.interdiff.txt4.15 KB
new tmgmt_2543294_delete_14.patch7.83 KB

I prefer to have early exits and streamline the remaining steps on the higher level without indentation.
Also fixed some unclean return value declaration and its minimal implementation.

Status: Needs review » Needs work

The last submitted patch, 14: tmgmt_2543294_delete_14.patch, failed testing.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new tmgmt_2543294_delete_16.patch7.85 KB
new tmgmt_2543294_delete_16.interdiff.txt4.16 KB

Oopsie, some accidental change. :-)

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Fixed

Committed.

  • miro_dietiker committed edb3475 on 8.x-1.x 
    Issue #2543294 by giancarlosotelo, miro_dietiker: Fatal error when...

Status: Fixed » Closed (fixed)

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