Query for open unclosed task items using wrong entity id [#2772807]

When checking for open task items in a local task, the query refers to task_item->id() instead of task->id().

Any correct result in the past has been pure coincidence.

      $unclosed_tasks = \Drupal::entityQuery('tmgmt_local_task_item')
        ->condition('tltid', $task_item->id())
        ->condition('status', LocalTaskItemInterface::STATUS_CLOSED, '<>')
        ->count()
        ->execute();

Comment	File	Size	Author
#14	tmgmt_2772807_task_14_test_only.patch	969 bytes	miro_dietiker
#14	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 175 pass
#12	unclosed_query_2772807_12_0.patch	1.61 KB	cgalli
#12	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 175 pass
#10	unclosed_query_2772807_7.patch	1.61 KB	cgalli
#10	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 174 pass, 1 fail
#3	unclosed_query_2772807.patch	680 bytes	cgalli
#3	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 175 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

26 July 2016 at 12:45

cgalli created an issue. See original summary.

Comment #2

cgalli CreditAttribution: cgalli commented 26 July 2016 at 12:46

Title:

Query for open unclosed task item using wrong entity id

» Query for open unclosed task items using wrong entity id

File	Size
unclosed_query_2772807.patch	680 bytes
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 175 pass

Comment #4

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 26 July 2016 at 12:51

I don't think that's enough, the condition is still on the task item id?

Will also need tests.

Comment #5

cgalli CreditAttribution: cgalli as a volunteer commented 26 July 2016 at 12:57

hmm. it takes the task from task_item and then the id of the task...

$task_item->getTask() returns the task to which the item belongs.

Comment #6

cgalli CreditAttribution: cgalli as a volunteer commented 26 July 2016 at 13:02

yes, I see...

will look at tests

Comment #7

miro_dietiker

Switzerland

CreditAttribution: miro_dietiker at MD Systems GmbH commented 26 July 2016 at 13:08

@Berdir isn't tltid the task ID? I think it is...

Comment #8

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 26 July 2016 at 13:43

right, the other is iid or so, I hate those ids :)

Comment #9

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 26 July 2016 at 13:44

However, as mentioned, the whole code block is the in the wrong place, just like job_item/job, this second part should live in a hook that listens on task items being closed. If there is ever another scenario where a task item can be closed then it wouldn't work.

Comment #10

cgalli CreditAttribution: cgalli as a volunteer commented 26 July 2016 at 14:32

File	Size
unclosed_query_2772807_7.patch	1.61 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 174 pass, 1 fail

With the patch, the query gets all task items that have the same task (tltid) and are not yet closed, then counts them. If none left, then the complete task can be closed. Correct?

The test checks that the task is not closed after saving only the first task item as completed. I am unsure if that is sufficient.

Comment #11

26 July 2016 at 14:39

Status:

Needs review

» Needs work

The last submitted patch, 10: unclosed_query_2772807_7.patch, failed testing.

Comment #12

cgalli CreditAttribution: cgalli as a volunteer commented 26 July 2016 at 15:09

File	Size
unclosed_query_2772807_12_0.patch	1.61 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 175 pass

Correct Syntax for the test....

Comment #13

cgalli CreditAttribution: cgalli as a volunteer commented 26 July 2016 at 16:04

Status:

Needs work

» Needs review

Comment #14

miro_dietiker

Switzerland

CreditAttribution: miro_dietiker at MD Systems GmbH commented 26 July 2016 at 20:12

File	Size
tmgmt_2772807_task_14_test_only.patch	969 bytes
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 175 pass

Submitting a test-only patch, hopefully see it fail.

Comment #15

miro_dietiker

Switzerland

CreditAttribution: miro_dietiker at MD Systems GmbH commented 26 July 2016 at 20:37

Status:

Needs review

» Needs work

A test-only patch needs to fail. otherwise it doesn't check what it's supposed to check: the current bug!

Plus yeah, the Berdir refactoring. :-)

Comment #16

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 27 July 2016 at 06:47

likely just needs to clear static cache and re-load the task to see the change.

If we don't already, we should also confirm that the task is closed at the end, but I hope we do that already

Comment #17

cgalli CreditAttribution: cgalli as a volunteer commented 27 July 2016 at 08:37

It depends.

The bug is such that it does not occur in all circumstances (see miros and christophes manual testing on 26 Jul), so the test will not always fail. But the test will make sure that the bug does not lead to false results.

IMHO there is no other possible test as the bug falsely closes the task without touching the items.

As berdir said, testing the task in the end for being closed. This is covered by:

    \Drupal::entityTypeManager()->getStorage('tmgmt_local_task_item')->resetCache();
    $task = tmgmt_local_task_load($task->id());
    $this->assertTrue($task->isClosed());
    list($first_task_item, $second_task_item) = array_values($task->getItems());
    $this->assertTrue($first_task_item->isClosed());
    $this->assertTrue($second_task_item->isClosed());