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.
Comment | File | Size | Author |
---|---|---|---|
#35 | 2992817-cron-35-interdiff.txt | 1.06 KB | tim.plunkett |
#35 | 2992817-cron-35.patch | 1.76 KB | tim.plunkett |
#34 | 2992817-34.patch | 1.83 KB | RoSk0 |
#34 | 2992817-29-34-interdiff.txt | 1.3 KB | RoSk0 |
#29 | 2992817-29.patch | 2.08 KB | phenaproxima |
Comments
Comment #2
yanniboi CreditAttribution: yanniboi at FreelyGive commentedPatch attached
Comment #3
yanniboi CreditAttribution: yanniboi at FreelyGive commentedComment #4
johnwebdev CreditAttribution: johnwebdev commentedGreat!
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.
Comment #5
alexpottLet's move the check to
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.
Comment #6
johnwebdev CreditAttribution: johnwebdev commentedMakes sense. Working on this.
Comment #7
johnwebdev CreditAttribution: johnwebdev commentedThis 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.
Comment #8
johnwebdev CreditAttribution: johnwebdev commentedComment #10
yanniboi CreditAttribution: yanniboi at FreelyGive commentedYay, 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.
Comment #11
johnwebdev CreditAttribution: johnwebdev commentedFollow up: #2992844: Add test coverage for InlineBlockUsage class
Comment #12
yanniboi CreditAttribution: yanniboi at FreelyGive commented@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?
Comment #13
tedbow@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!
Comment #14
tedbow#2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder was recommitted! 🎉
Retesting last patch. If passes I think RTBC
Comment #16
tim.plunkett🤔
(the rest looks fine)
Comment #17
johnwebdev CreditAttribution: johnwebdev commented#16 @tim.plunkett An assertion is needed for the test to be executed. And deleteUsage does not return anything, so cannot assert against that.
Comment #18
tim.plunkettThen 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();
Comment #19
RoSk0Converted to unit and added assertion as per #18.
Comment #21
RoSk0Back to 'needs review' after random fail
Comment #22
johnwebdev CreditAttribution: johnwebdev commentedPlease 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
Comment #23
johnwebdev CreditAttribution: johnwebdev commentedComment #24
RoSk0I 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.
Comment #25
mpotter CreditAttribution: mpotter at Phase2 commentedWe are using this on a site and it fixed our cron issues. The patch seems really straightforward. Tests are passing.
Comment #26
phenaproximaHere is a fail patch for #19. If it fails, +1 RTBC for that.
Comment #27
tim.plunkettGoing to need to reup the real patch as the last one.
Comment #29
phenaproximaDone. Kicking directly to RTBC because this is just a re-upload of #19, the fail patch has failed, and @mpotter and I already RTBCed.
Comment #31
phenaproximaFail patch failed. Back to RTBC.
Comment #32
dat deaf drupaler CreditAttribution: dat deaf drupaler as a volunteer commentedI can confirm patch #29 works for me.
Comment #33
tim.plunkettI'm surprised this doesn't fail for not having any assertions
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?
Comment #34
RoSk0Would 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...
Comment #35
tim.plunkett\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.
Comment #36
RoSk0That looks great! Thanks Tim.
Comment #37
alexpottCommitted and pushed 964372f57a to 8.7.x and e3829e1fae to 8.6.x. Thanks!