Contrib.social

Subtle wizard pagination bug posts/pages, remove "empty"

Open on Drupal.org →
Created on 26 November 2024, 3 months ago

Problem/Motivation

We should try to remove all "empty" statements and also fix the comparators flagged by phpstan as using mixed inputs into booleans. I already fixed a checkbox that had this kind of issue. All boolean comparators should be === and not be using soft type shifts. However, I don't know the quick fix for this one.

(There are other "empty" in the WordPressMigrationGenerator.php)

    // Setup the content migrations.
    foreach (['post', 'page'] as $wordpress_type) {
      if (!empty($this->configuration[$wordpress_type]['type'])) {
        $this->createContentMigration($wordpress_type);
      }
    }
  }

If you look carefully at the stages of the wizard "posts" and "pages" shift around a little bit when you are proceeding thru the stages. There is a statement flagged by phpstan as using "empty" which I think assigns them their slots. I briefly tried to change it and the form stopped working as it should. This might be a good task for someone looking for a minor form API / ctools patch. (I am not an expert in the ctools wizard API.) It is not a show stopper but it is a little below Drupal coding standards.

So, the leading example:

https://git.drupalcode.org/project/wordpress_migrate/-/blob/8.x-3.x/word...

    // Dynamically add the content migration(s) that have been configured by
    // ContentSelectForm.
    if (!empty($cached_values['post']['type'])) {
      $steps += [
        'blog_post' => [
          'form' => 'Drupal\wordpress_migrate_ui\Form\ContentTypeForm',
          'title' => $this->t('Posts'),
          'values' => ['wordpress_content_type' => 'post'],
        ],
      ];
    }
    if (!empty($cached_values['page']['type'])) {
      $steps += [
        'page' => [
          'form' => 'Drupal\wordpress_migrate_ui\Form\ContentTypeForm',
          'title' => $this->t('Pages'),
          'values' => ['wordpress_content_type' => 'page'],
        ],
      ];
    }

Proposed resolution

Change to fully compliant code.

Remaining tasks

Patch to fix the notice

User interface changes

Acts as it should.

API changes

Possible CTools wizard paging change.

Data model changes

None.

🐛 Bug report
Status

Active

Version

3.0

Component

User interface

Created by

🇺🇸United States hongpong Philadelphia

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @hongpong
  • First commit to issue fork.
  • Comment 3 months ago →
    🇳🇮Nicaragua baltowen
  • Comment 3 months ago →
    🇺🇸United States hongpong Philadelphia
  • Comment 3 months ago →
    🇺🇸United States hongpong Philadelphia
  • Comment 3 months ago →
    🇺🇸United States hongpong Philadelphia
  • Merge request !34Resolve #3489999 "Subtle wizard pagination" → (Open) created by baltowen
  • Pipeline finished with Failed
    3 months ago
    Total: 145s
    #351305
  • Comment 3 months ago →
    🇳🇮Nicaragua baltowen

    I worked on removing the empty statements.

    I noticed an error logic in the code. When selecting the content types, if only pages were selected it would trigger an error that at least one content type should be selected. The error message suggests that it should not be possible to have an import without content types, so I implemented that. If we want to allow that, we can remove the validateForm method altogether.

    I also fixed a coding standard issue. It seems there are still static calls to Drupal. Not sure how to address those.

  • Comment 3 months ago →
    🇺🇸United States hongpong Philadelphia

    thanks baltowen very good stuff. Regarding the equals signs, there are some silly rules in phpcs that already exist. From today's HEAD (not your patch). I

    Drupal.Formatting.MultipleStatementAlignment.NotSame
    Drupal.Formatting.MultipleStatementAlignment.Incorrect

     197 | ERROR | [x] Equals sign not aligned correctly; expected 1 space but
     |       |     found 16 spaces
     |       |     (Drupal.Formatting.MultipleStatementAlignment.Incorrect)
 261 | ERROR | [x] Equals sign not aligned with surrounding assignments;
     |       |     expected 21 spaces but found 1 space
     |       |     (Drupal.Formatting.MultipleStatementAlignment.NotSame)
 267 | ERROR | [x] Equals sign not aligned with surrounding assignments;
     |       |     expected 27 spaces but found 1 space
     |       |     (Drupal.Formatting.MultipleStatementAlignment.NotSame)

    I think the alignments are very silly for them to impose on us. phpcs.xml.dist (or maybe without .dist) could perhaps override this in Drupal flavored GitlabCI within this project?
    Using statements like https://git.drupalcode.org/project/drupal/blob/8.8.x/core/phpcs.xml.dist

      <rule ref="Drupal.Arrays.Array">
    <!-- Sniff for these errors: CommaLastItem -->
    <exclude name="Drupal.Arrays.Array.ArrayClosingIndentation"/>
    <exclude name="Drupal.Arrays.Array.ArrayIndentation"/>
    <exclude name="Drupal.Arrays.Array.LongLineDeclaration"/>
  </rule>

    see #2994956: Excluding sniffs from phpcs →

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024