Problem/Motivation

Migration missing for global settings.

Steps to reproduce

NA

Proposed resolution

Add migrations.

Remaining tasks

NA

User interface changes

NA

API changes

NA

Data model changes

NA

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

srishti.bankar created an issue. See original summary.

srishtiiee’s picture

srishtiiee’s picture

StatusFileSize
new3.44 KB
new920 bytes

Corrected Kernel test for configuration migration.

wim leers’s picture

I just have a bunch of coding style nitpicks — these are all things that in Drupal core we comply with 🙈🤓

Looking great otherwise!

  1. +++ b/tests/src/Kernel/MigrateEntityPrintTest.php
    @@ -0,0 +1,48 @@
    +  protected static $modules = [
    +    'entity_print',
    +  ];
    

    Nit: missing docblock, and missing newlines before and after.

  2. +++ b/tests/src/Kernel/MigrateEntityPrintTest.php
    @@ -0,0 +1,48 @@
    +  }
    +
    +
    +
    +  /**
    

    Nit: 3 newlines, should be one.

  3. +++ b/tests/src/Kernel/MigrateEntityPrintTest.php
    @@ -0,0 +1,48 @@
    +  public function testMigration() {
    

    Nit: could also have the void return type!

  4. +++ b/tests/src/Kernel/MigrateEntityPrintTest.php
    @@ -0,0 +1,48 @@
    +    $this->assertSame(true, $config_before->get('default_css'));
    

    Nit: in Drupal we always write TRUE and FALSE (all uppercase).

  5. +++ b/tests/src/Kernel/MigrateEntityPrintTest.php
    @@ -0,0 +1,48 @@
    +    $this->executeMigration('entity_print_settings');
    +
    +
    +    $config_after = $this->config('entity_print.settings');
    

    Nit: two newlines, should be one, not two.

  6. +++ b/tests/src/Kernel/MigrateEntityPrintTest.php
    @@ -0,0 +1,48 @@
    +  }
    +}
    

    Nit: should have one newline between these.

srishtiiee’s picture

StatusFileSize
new3.58 KB
new1.34 KB

.

srishtiiee’s picture

StatusFileSize
new3.58 KB
wim leers’s picture

Status: Needs review » Needs work
+++ b/tests/src/Kernel/MigrateEntityPrintTest.php
@@ -0,0 +1,52 @@
+class MigrateEntityPrintTest extends MigrateDrupal7TestBase {
+
+    /**
+     * @var string[]
+     */
+    protected static $modules = [
+        'entity_print',
+    ];
+
+    /**
+     * {@inheritdoc}
+     */
+    protected function setUp(): void {
+        parent::setUp();
+        $this->loadFixture(implode(DIRECTORY_SEPARATOR, [

One more nitpicking round: this should be formatted using 2 spaces for each indentation level, not 4 😅

srishtiiee’s picture

Status: Needs work » Needs review
StatusFileSize
new3.51 KB
srishtiiee’s picture

srishtiiee’s picture

StatusFileSize
new3.52 KB
wim leers’s picture

Status: Needs review » Needs work

Last round probably! :D

  1. +++ b/tests/src/Kernel/MigrateEntityPrintTest.php
    @@ -0,0 +1,53 @@
    +  /**
    +   * Enables required modules
    +   * @var string[]
    +   */
    

    Nit: can just be {@inheritdoc} 🤓

  2. +++ b/tests/src/Kernel/MigrateEntityPrintTest.php
    @@ -0,0 +1,53 @@
    +    $this->installConfig(['entity_print']);
    +
    +  }
    

    Nit: should have no newline after the last statement and before the closing brace.

  3. +++ b/tests/src/Kernel/MigrateEntityPrintTest.php
    @@ -0,0 +1,53 @@
    +  public function testMigration() : void {
    

    This return type hint has a space before the colon. The earlier one does not. Let's be consistent.

    (Drupal core does not have a standardized style for this yet, but the majority have no space before it.

  4. +++ b/tests/src/Kernel/MigrateEntityPrintTest.php
    @@ -0,0 +1,53 @@
    +    $this->assertSame(FALSE, $config_after->get('default_css'));
    +
    +  }
    

    Same here.

karishmaamin’s picture

Assigned: Unassigned » karishmaamin
karishmaamin’s picture

Assigned: karishmaamin » Unassigned
Status: Needs work » Needs review
StatusFileSize
new3.47 KB
new910 bytes

Fix for #11 issues

wim leers’s picture

Status: Needs review » Reviewed & tested by the community

Looking great!

wim leers’s picture

Status: Reviewed & tested by the community » Needs review

@srishti.bankar just pointed out there are still a few unaddressed coding standards issues 🙈

srishtiiee’s picture

StatusFileSize
new3.48 KB
new450 bytes

Fixed all the coding standard issues.

wim leers’s picture

Status: Needs review » Reviewed & tested by the community

👍

huzooka’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/migrations/entity_print_settings.yml
@@ -0,0 +1,15 @@
+  default_css: entity_print_default_css

Drupal 7 Entity Print does not set a value for its entity_print_default_css variable. This means that the variable might be completely missing from the Drupal 7 varaible table. Based on the codebase of the module, the variable's default value is TRUE.

The problem is that if the variable migrate source plugin doesn't find a value for the given variables, it will return with a row without the entity_print_default_css source prop being set. In this case, the migration system will set the default_css destination property to NULL, and when the entity_print.settings is being saved, schema api will transform its value to FALSE.

Solution: use the default_value process plugin with its strict config set to TRUE:

[...]
process:
  default_css: 
    plugin: default_value
    strict: true
    default_value: true
    source: entity_print_default_css
[...]
srishtiiee’s picture

Status: Needs work » Needs review
StatusFileSize
new3.56 KB
new538 bytes

Made the required changes 👍🏼

huzooka’s picture

Status: Needs review » Reviewed & tested by the community

Great!

wim leers’s picture

Thanks, @huzooka! 😊🙏

imalabya made their first commit to this issue’s fork.

vladimiraus’s picture

Version: 8.x-2.4 » 8.x-2.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks.

Status: Fixed » Closed (fixed)

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