🇺🇸United States @trackleft2

Tucson, AZ 🇺🇸
Account created on 2 April 2014, about 11 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

trackleft2 changed the visibility of the branch 3505464-the-nodetype-plugin to hidden.

🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

I think this fix warrants a new minor release of this module (Since this is a non backwards compatible breaking change).

See https://www.drupal.org/node/2983299 for Drupal core version compatibility.

Alternatively someone could write a patch that is compatible with the drupal core versions noted in the .info.yml file. ^10.3 || ^11.0

🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

I'm not sure what is going on with the PHPUnit test, though there is currently an issue open for this: 🐛 Fix PHPUnit tests: "FieldGroupUiTest" is randomly failing Active

🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

@naidim I've opened a new issue for this #3521275 where I'll add a patch and move this merge request.

🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

@naidim as far as I can tell, there are two options.

Option 1: Create a sub module with just the library that has a dependency on field_ui
Option 2: Use hook_library_info_alter to add the dependency if the field_ui module is enabled.

🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

Once this is merged, I suggest adding this to the project page at https://drupal.org/project/search_exclude

<h3>Views integration</h3>

<p>
  The Search Exclude module integrates with Views by providing:
</p>
<ul>
  <li>A <strong>Search Keywords</strong> filter using the <code>search_keywords

handler

  • A Score field using the search_score handler, which can also be used to sort results by relevance
  • This allows you to create Views that behave like Drupal core search results, while excluding specific content types or nodes using Search Exclude.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    Added a new PHPUnit test and set the gitlab-CI to fail on cspell errors.

    Added a cspell rule to handle this issue 📌 Either avoid or explicitly test binary encoding in default configuration Needs work

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    trackleft2 changed the visibility of the branch 3.x to hidden.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    trackleft2 changed the visibility of the branch 2.0.x to hidden.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

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

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    I was able to get the tests working again with all of the upstream changes merged in.

    If you check the "Test only changes" job, you can see the old broken functionality in action.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    trackleft2 changed the visibility of the branch 3491233-drupal-10.4-rc1-3.x to hidden.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    trackleft2 changed the visibility of the branch 3451207-automated-drupal-11 to hidden.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    trackleft2 changed the visibility of the branch project-update-bot-only to hidden.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    Merged and available in 2.1.2

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    Merged and available in 2.1.2

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    trackleft2 changed the visibility of the branch 3451551-fix-the-logging to hidden.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    Thanks for all of the review on this!

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    Just realized we need to set the new configuration option by adding something to https://git.drupalcode.org/project/field_group/-/blob/4.x/field_group.po...

    Maybe something like

    /**
     * Set default 'remove_whitespace' setting for HtmlElement formatters.
     */
    function field_group_post_update_add_remove_whitespace_setting(array &$sandbox = NULL): void {
      $entity_type_manager = \Drupal::entityTypeManager();
      $storage = $entity_type_manager->getStorage('field_group');
    
      if (!isset($sandbox['group_ids'])) {
        $group_ids = $storage->getQuery()
          ->accessCheck(FALSE)
          ->execute();
        $sandbox['group_ids'] = array_values($group_ids);
        $sandbox['current'] = 0;
        $sandbox['total'] = count($sandbox['group_ids']);
      }
    
      $limit = 25;
      $group_ids_chunk = array_slice($sandbox['group_ids'], $sandbox['current'], $limit);
      $sandbox['current'] += count($group_ids_chunk);
    
      /** @var \Drupal\field_group\Entity\FieldGroup[] $groups */
      $groups = $storage->loadMultiple($group_ids_chunk);
      foreach ($groups as $group) {
        $settings = $group->get('settings');
        if (($settings['formatter'] ?? '') === 'html_element' && !isset($settings['remove_whitespace'])) {
          $settings['remove_whitespace'] = FALSE;
          $group->set('settings', $settings);
          $group->save();
        }
      }
    
      $sandbox['#finished'] = ($sandbox['current'] >= $sandbox['total']);
    }
    
    
    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    Overall I like what you've done here, thank you!
    I've added some suggestions to the wording of the setting that you can decide whether you like or not.

    I've marked this as RTBC, since my code suggestions are optional.

    I do wonder if people will be confused by this new strange option that doesn't really have an explanation.

    Essentially, the "spaceless" filter exists because browsers interpret the space between HTML elements.

    Example:

    This

    <div>test1</div>
    <div>test2</div>
    

    behaves differently from

    <div>test1</div><div>test2</div>
    

    If a developer writes a template for code readability, they risk adding in browser interpreted breaking space.

    This is in essence a coding standards artifact.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    I'll leave this for someone else to mark RTBC

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    CSpell job passed!

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

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

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    I can merge it, no problem. Thanks for the approval.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

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

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    trackleft2 created an issue.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    Actually, I might be wrong about this, because I saw this on a different issue.:

    With this removal of any Composer constraint on Drupal core, this will mean that Composer will happily allow installing this version regardless of the Composer requirement for Drupal core. (If this module didn't have a composer.json file at all, then drupal.org packaging would use the info.yml file to generate Composer constraints, but since there is a composer.json file, this doesn't happen).

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    IMHO, the real solution would be to update the menu_block.post_update.php file with a new function to add the new key for sites that installed the module prior to the new key existing.

    See https://git.drupalcode.org/project/menu_block/-/blob/8.x-1.x/menu_block.... for an extremely similar example.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    Possible scenarios where cache should be invalidated:
    - When the config_sync update mode has changed.
    - When new managed configuration files appear or are removed from the codebase.
    - When active configuration changes.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    trackleft2 changed the visibility of the branch 3439698-add-configdistro-status to hidden.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    See the Merge Request on 🐛 Not compatible with search_current_language module Active with a PHPUnit test that depends on another drupal module via the composer.json.

    Additionally, we don't have to set the version constraint in composer.json, we can leave that off if necessary.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    If this is merged it would be good to add the new feature to the release notes and a scenario or two in which to use the setting.
    How do we expect this to work, when the paths setting is negated?

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    I haven't been able to figure this out.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    trackleft2 changed the visibility of the branch 3431903-automated-drupal-11-trackleft2 to hidden.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    Oops, I saw the composer.json when I downloaded the repo.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    While working on other issues I found that this module does not contain a composer.json which makes it difficult to work in the way that I would like to locally. For example here is my .lando.yml file:

    name: media_migration
    recipe: drupal10
    config:
      php: '8.3'
      via: apache:2.4
      webroot: web
      database: mariadb:10.6
      xdebug: false
    services:
      database:
        type: compose
        services:
          image: mariadb:10.6
          command: docker-entrypoint.sh mariadbd
          restart: always
          ports:
            - '3306'
          environment:
            MARIADB_ALLOW_EMPTY_ROOT_PASSWORD: 'true'
            MARIADB_DATABASE: drupal10
            MYSQL_DATABASE: drupal10
            MARIADB_USER: drupal10
            MARIADB_PASSWORD: drupal10
      appserver:
        overrides:
          environment:
            SIMPLETEST_DB: 'mysql://drupal10:drupal10@database/drupal10'
            SIMPLETEST_BASE_URL: 'http://appserver'
          volumes:
            # Don't share our host working directory as /app. We want /app empty for composer.
            - /app
            # Instead share our host working directory as a standalone package.
            - .:/usr/local/media_migration
        build:
          # Create a new Drupal project and use the module as a non-packagist repository.
          - composer create-project --dev drupal/recommended-project:11.1.x /app
          - composer config extra.enable-patching true
          - composer config extra.composer-exit-on-patch-failure true
          - composer config allow-plugins.cweagans/composer-patches true
          - composer require cweagans/composer-patches
          - composer config minimum-stability dev
          - composer config allow-plugins.phpstan/extension-installer true
          - composer require --dev drupal/core-dev:^11.1 drush/drush phpspec/prophecy-phpunit:* phpstan/extension-installer mglaman/phpstan-drupal phpstan/phpstan-deprecation-rules drupal/config_inspector drupal/devel drupal/gin
          - composer config repositories.localdev path /usr/local/media_migration && composer require drupal/media_migration:\*@dev
      node:
        type: node:20
        build:
          - yarn install
      chromedriver:
        type: compose
        services:
          image: seleniarm/standalone-chromium:4.1.4-20220429
          command: /opt/bin/entry_point.sh
    tooling:
      # Provide a command to install Drupal.
      install:
        service: appserver
        cmd:
          - /app/vendor/bin/drush --root=/app/web site:install --account-mail=noreply@example.com --account-name=admin --account-pass=admin --db-url=mysql://drupal10:drupal10@database:3306/drupal10 -y --verbose
          - /app/vendor/bin/drush en -y media_migration devel config_inspector
      # Provide Drush tooling to automatically know the Drupal root.
      drush:
        service: appserver
        cmd: /app/vendor/bin/drush --root=/app/web
      phpcs:
        service: appserver
        cmd: /app/vendor/bin/phpcs -s --colors --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml web/modules/contrib/media_migration
      # Provide PHPCBF tooling to fix coding standards.
      phpcbf:
        service: appserver
        cmd: /app/vendor/bin/phpcbf -s --colors --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml web/modules/contrib/media_migration
      # Provide phpstan tooling to check for code quality and deprecated code.
      phpstan:
        service: appserver
        cmd: /app/vendor/bin/phpstan analyse --configuration web/modules/contrib/media_migration/phpstan.neon web/modules/contrib/media_migration
      # Provide phpunit tooling to run unit tests.
      phpunit:
        service: appserver
        cmd: /app/vendor/bin/phpunit --configuration /app/web/core/phpunit.xml.dist --bootstrap /app/web/core/tests/bootstrap.php /app/web/modules/contrib/media_migration
      yarn:
        service: node
        cmd: yarn
      eslint:
        service: node
        cmd: yarn run eslint --color
    
    
    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    One thing that I am not clear on is:
    If a recipe alters a single line within config
    Does that make the entire configuration overridden or can I merge upstream changes around the recipe provided config.

    🇺🇸United States trackleft2 Tucson, AZ 🇺🇸

    I wonder if we should address the recipe question on the project page.

    Production build 0.71.5 2024