Problem/Motivation

Continuing from https://www.drupal.org/project/drupal/issues/2951715#comment-12559059
When there is no php or request_path specified in the visibilty configuration, block_visibility plugin skips that particular row, But currently there is no where these exception get logged.

core/modules/block/src/Plugin/migrate/process/BlockVisibility.php:118:          
throw new MigrateSkipRowException();

Proposed resolution

Log a message to MigrateSkipRowException to clearly explain why the row is skipped.

Remaining tasks

Patch

User interface changes

NA

API changes

NA

Data model changes

NA

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rakesh.gectcr created an issue. See original summary.

rakesh.gectcr’s picture

Issue summary: View changes
rakesh.gectcr’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.9 KB
rakesh.gectcr’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 3: 2959087-3.patch, failed testing. View results

rakesh.gectcr’s picture

Issue tags: +Nwdug_may18
jofitz’s picture

Status: Needs work » Needs review
FileSize
2.26 KB

No interdiff because I started from scratch.

quietone’s picture

  1. +++ b/core/modules/block/src/Plugin/migrate/process/BlockVisibility.php
    @@ -115,7 +115,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
    +          throw new MigrateSkipRowException(sprintf("The block will have no PHP or request_path visibility configuration for destination '%s'.", $destination_property));
    

    Can this include more information to identity the block? Say the module and bid? I am not sure what is best.

  2. +++ b/core/modules/block/tests/src/Unit/Plugin/migrate/process/BlockVisibilityTest.php
    @@ -80,4 +81,15 @@ public function testTransformPhpDisabled() {
    +    $transformed_value = $this->plugin->transform([2, '<?php', []], $this->migrateExecutable, $this->row, 'destinationproperty');
    

    $transformed_value is not needed

quietone’s picture

Status: Needs review » Needs work

Ah, the is Needs Work too,

jofitz’s picture

Status: Needs work » Needs review
FileSize
2.71 KB
2.67 KB

Addressed #8.

quietone’s picture

Status: Needs review » Needs work

Applied that patch, modified BlockVisibilty plugin to always throw the exception and then ran the d6_block migration to see the error messages.
The block with bid '12' from module 'block' will have no PHP or request_path visibility configuration for destination 'visibility'.
The bid and module really help and will be useful for anyone chasing down a problem. After seeing 24 of these message in the message table I realize that the phrase 'for destination visibility' doesn't add value. The destination will always be 'visibility', it is the defined property name in the Block entity and can't be anything else. So, I think that needs to be removed and then the message is just "The block with bid '12' from module 'block' will have no PHP or request_path visibility configuration.".

Sorry I didn't see that sooner!

jofitz’s picture

Status: Needs work » Needs review
FileSize
2.27 KB
2.61 KB

Improved the Exception message.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Assuming this comes back green, let's get this merged. All feedback is addressed.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 90870fe81c to 8.7.x and c3cd6fc4e5 to 8.6.x. Thanks!

  • alexpott committed 90870fe on 8.7.x
    Issue #2959087 by Jo Fitzgerald, rakesh.gectcr, quietone: Log message if...

  • alexpott committed c3cd6fc on 8.6.x
    Issue #2959087 by Jo Fitzgerald, rakesh.gectcr, quietone: Log message if...

Status: Fixed » Closed (fixed)

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