Hello!

The webform block (WebformBlock) does not report any dependency, but in fact it does depend on the webform config entity.

Failure to report its dependency may lead to a situation when the dependent webform is removed and the block then has difficult time figuring out what to do. If the dependency is reported, then core configuration API will not allow a webform to be deleted before all of its blocks are removed too.

Supplying a patch shortly.

CommentFileSizeAuthor
#12 interdiff-10-12.txt1.91 KBbucefal91
#12 2944515-block-dependencies-12.patch9.29 KBbucefal91
#10 2944515-block-dependencies-9_.patch7.38 KBbucefal91
#10 interdiff-6-9.txt813 bytesbucefal91
#6 interdiff-4-6.txt719 bytesbucefal91
#6 2944515-block-dependencies-6.patch7.81 KBbucefal91
#4 2944515-block-dependencies-4.patch6.39 KBbucefal91
#4 interdiff-2-4.txt1.75 KBbucefal91
#2 2944515-block-dependencies-2.patch5.16 KBbucefal91
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bucefal91 created an issue. See original summary.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
Status: Active » Needs review
FileSize
2944515-block-dependencies-2.patch5.16 KB

This patch fixes the bug + covers it with a unit test.

I shall additionally:

  1. Clean up the WebformBlock code so it no longer checks whether webform exists. With this bug resolved it is guaranteed to exist (if it is removed, then the block is removed too)
  2. Write an update hook to propagate this dependency into all existing blocks.

Status: Needs review » Needs work

The last submitted patch, 2: 2944515-block-dependencies-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
Status: Needs work » Needs review
FileSize
interdiff-2-4.txt1.75 KB
2944515-block-dependencies-4.patch6.39 KB

We should see 100% pass now.

I also addressed both items from my previous comment - update hook (tested manually and confirm it works), and cleaning up WebformBlock from checking whether $webform exists.

Status: Needs review » Needs work

The last submitted patch, 4: 2944515-block-dependencies-4.patch, failed testing. View results

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
Status: Needs work » Needs review
FileSize
2944515-block-dependencies-6.patch7.81 KB
interdiff-4-6.txt719 bytes

Alright, now yes, let the show start :) I've fixed the failing test.

My changes in WebformBlock made it a little stricter about block initialization (you can't save a block until you have defined its webform) - this patch fixes up the failing test so it specifies the webform at the moment of block creation and not afterwords as it was doing before.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz at The Big Blue House commented
Status: Needs review » Reviewed & tested by the community 
+++ b/tests/src/Unit/Plugin/views/field/WebformSubmissionBulkFormTest.php
@@ -12,7 +12,7 @@ use Drupal\webform\Plugin\views\field\WebformSubmissionBulkForm;
- * @group webform

This is causing a merge conflict.

Otherwise, the patch looks good.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented

I also have 1 extra thing. It popped up in my head after I switched off the computer yesterday. The current update hook will fail with fatal error if there is already inconsistency in the DB, i.e. if there is already a block whose webform has been deleted. I will wrap the re-saving of the $block with checking whether it's webform exists. If it exists I will proceed to re-saving, but if it doesn't, I will instead delete the block since there is no use of it when it's not backed by any existing webform.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz at The Big Blue House commented

Can you please also update exported blocks that included in the below test modules?

  • webform_test_block_context
  • webform_test_block_custom
  • webform_test_block_submission_limit
bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
Status: Reviewed & tested by the community » Needs review
FileSize
interdiff-6-9.txt813 bytes
2944515-block-dependencies-9_.patch7.38 KB

My #8 is addressed in this patch. I've just tested the new update hook manually having 2 blocks: one block was fine (backed by an existing webform) and another was corrupted (the webform was removed while keeping the block around).

After running the update hook the corrupted block is removed from DB without any error. The OK block is successfully re-saved to acknowledge drupal core of its correct dependencies.

As far as the minor conflict - I rebased my local branch and the patch now does not contain the conflict.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented

Just saw your comment. Yup. I'll look into those.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
FileSize
2944515-block-dependencies-12.patch9.29 KB
interdiff-10-12.txt1.91 KB

I scanned all the blocks stored in /config folders of all the (sub-)modules and only 2 blocks were built on "webform_block" plugin. This patch updates those 2 blocks with proper dependencies.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz at The Big Blue House commented
Status: Needs review » Reviewed & tested by the community

  • bucefal91 committed f9c2e22 on 8.x-5.x 
    Issue #2944515 by bucefal91: Reporting proper dependencies for webform...
bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
Status: Reviewed & tested by the community » Fixed

Committed :)

Status: Fixed » Closed (fixed)

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