Problem/Motivation

This is a followup for changes introduced by #2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder.

The layout builder cron job removes unused inline blocks. It however errors when there are no inline blocks to be removed as it tries to run a query with an 'IN' condition passing it an empty array.

Drupal\Core\Database\InvalidQueryException: Query condition 'block_content_id IN ()' cannot be empty. in Drupal\Core\Database\Query\Condition->condition() (line 103 of /home/contacts/domains/devb.contacts.freelygive.org.uk/app/web/core/lib/Drupal/Core/Database/Query/Condition.php).

Proposed resolution

Check if the list of blocks to be removed is empty before running the query.

Remaining tasks

  • Add empty check.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yanniboi created an issue. See original summary.

yanniboi’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
722 bytes

Patch attached

yanniboi’s picture

Issue summary: View changes
johnwebdev’s picture

Great!

We already have test coverage for running cron when blocks should be deleted that will verify that this change does not break anything else, but yeah it's really trivial and won't, so I'll RTBC once patch is green.

alexpott’s picture

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

Let's move the check to

  /**
   * Delete the inline blocks' the usage records.
   *
   * @param int[] $block_content_ids
   *   The block content entity IDs.
   */
  public function deleteUsage(array $block_content_ids) {
    $query = $this->database->delete('inline_block_usage')->condition('block_content_id', $block_content_ids, 'IN');
    $query->execute();
  }

if we wrapped the code in here with if (!empty($block_content_ids)) { then all code calling this with an empty array wouldn't error.

Plus I just noticed we're missing a test of InlineBlockUsage class - let's add a KernelTest here so we don't need to mock the db.

johnwebdev’s picture

Makes sense. Working on this.

johnwebdev’s picture

This patch moves the empty check to deleteUsage(). Also adds a test to verify an empty array call can be done which covers the bug. I'll create a follow-up issue to get full coverage of the class.

johnwebdev’s picture

Status: Needs work » Needs review

The last submitted patch, 7: 2992817-7--test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

yanniboi’s picture

Yay, that's working as expected.

I think once we have the follow up issue for the rest of the test coverage, we can mark this issue as RTBC.

johnwebdev’s picture

yanniboi’s picture

@johndevman, the original issue #2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder has been reverted because of security issues. Shall we move our patches back there so it all gets committed together?

tedbow’s picture

@yanniboi #2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder was reverted because of a security issue so it should only be focused on the security issue for the recommit to make sure it gets taken care of.

This issue could get in right after. Nice catch BTW!

tedbow’s picture

Issue tags: -Needs tests

#2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder was recommitted! 🎉

Retesting last patch. If passes I think RTBC

The last submitted patch, 7: 2992817-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/core/modules/layout_builder/tests/src/Kernel/InlineBlockUsageTest.php
@@ -0,0 +1,46 @@
+    $this->assertTrue(TRUE);

🤔

(the rest looks fine)

johnwebdev’s picture

#16 @tim.plunkett An assertion is needed for the test to be executed. And deleteUsage does not return anything, so cannot assert against that.

tim.plunkett’s picture

Then either a comment should be added to explain that, or an assertion could be written.
For example, mock the database connection and add $this->database->delete('inline_block_usage')->shouldNotBeCalled();

RoSk0’s picture

Version: 8.6.x-dev » 8.7.x-dev
Status: Needs work » Needs review
FileSize
2.08 KB
1.79 KB

Converted to unit and added assertion as per #18.

Status: Needs review » Needs work

The last submitted patch, 19: 2992817-19.patch, failed testing. View results

RoSk0’s picture

Status: Needs work » Needs review

Back to 'needs review' after random fail

johnwebdev’s picture

Status: Needs review » Needs work

Please correct me If I'm wrong, but looking at the dispatcher the test here was never executed. See #17 and #18.

Edit: Looks like it was. Back to needs review.

Edit: Actually, I'm not entirely wrong because this test would never execute on PHPUnit 6:
https://github.com/sebastianbergmann/phpunit/issues/2484

johnwebdev’s picture

Status: Needs work » Needs review
RoSk0’s picture

I would not agree, test ran and run correctly. Search for
19:25:16 Drupal\Tests\layout_builder\Unit\InlineBlockUsageTest 1 passes
in the console output. No assertions doesn't mean that test is broken. Presence of expectations compensate this.

mpotter’s picture

Status: Needs review » Reviewed & tested by the community

We are using this on a site and it fixed our cron issues. The patch seems really straightforward. Tests are passing.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.29 KB

Here is a fail patch for #19. If it fails, +1 RTBC for that.

tim.plunkett’s picture

Going to need to reup the real patch as the last one.

Status: Needs review » Needs work

The last submitted patch, 26: 2992817-26-FAIL.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.08 KB

Done. Kicking directly to RTBC because this is just a re-upload of #19, the fail patch has failed, and @mpotter and I already RTBCed.

The last submitted patch, 26: 2992817-26-FAIL.patch, failed testing. View results

phenaproxima’s picture

Fail patch failed. Back to RTBC.

dat deaf drupaler’s picture

I can confirm patch #29 works for me.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Blocks-Layouts

I'm surprised this doesn't fail for not having any assertions

+++ b/core/modules/layout_builder/tests/src/Unit/InlineBlockUsageTest.php
@@ -0,0 +1,47 @@
+    $connection = $this->createMock(Connection::class);
+
+    $connection
+      ->expects($this->never())
+      ->method('delete')
+      ->with('inline_block_usage');

This should be moved to the test method, if this class were ever expanded it's unlikely this mock would be appropriate.

Can this be switched to prophesy?

RoSk0’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
1.83 KB

Would probably agree with the point of extension, but prophesy, why? When we have all the assertions in the world we can lazily use prophesy imho, but when we testing by expectations...

tim.plunkett’s picture

\PHPUnit_Framework_MockObject_MockObject is not used anywhere in Layout Builder tests and I'd prefer the consistency.

Also fixed the lowerCamelCase by removing the local variable.

RoSk0’s picture

Status: Needs review » Reviewed & tested by the community

That looks great! Thanks Tim.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 964372f57a to 8.7.x and e3829e1fae to 8.6.x. Thanks!

  • alexpott committed 964372f on 8.7.x
    Issue #2992817 by RoSk0, johndevman, phenaproxima, tim.plunkett,...

  • alexpott committed e3829e1 on 8.6.x
    Issue #2992817 by RoSk0, johndevman, phenaproxima, tim.plunkett,...

Status: Fixed » Closed (fixed)

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