Environmental indicator incorrect version identifier

Created on 16 December 2024, 6 days ago

Problem/Motivation

Updating to version 4.0.20 causes a crash when reloading due to a null value being passed instead of string to the function `getVersionIdentifier(string $type)`

Steps to reproduce

Upgrade module, update database, cache clear, reload page.

Proposed resolution

Change line 17 of the install to include a dot, not an underscore.
$settings->set('version_identifier', 'environment_indicator.current_release');

πŸ› Bug report
Status

Active

Version

4.0

Component

Code

Created by

πŸ‡³πŸ‡ΏNew Zealand atowl

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

Merge Requests

Comments & Activities

  • Issue created by @atowl
  • Merge request !81Update file environment_indicator.install β†’ (Open) created by atowl
  • πŸ‡³πŸ‡ΏNew Zealand atowl
  • Pipeline finished with Success
    6 days ago
    Total: 135s
    #369326
  • heddn Nicaragua

    Fixes the issue.

  • πŸ‡ͺπŸ‡ΈSpain isholgueras

    Maybe I'm missing something but I don't know how this patch could solve the issue.

    For the value for version_identifier could be environment_indicator_current_release
    https://git.drupalcode.org/project/environment_indicator/-/blob/4.x/conf...

    The available options for the version_identifier are:
    - environment_indicator_current_release
    - deployment_identifier
    - drupal_version
    - none

    https://git.drupalcode.org/project/environment_indicator/-/blob/4.x/src/...

    but not environment_indicator.current_release

    I'm also unable to reproduce the error.

    From what version are you updating? 4.0.19 or earlier? What error do you get when you update from 4.0.19 to 4.0.20?

    Thanks!

  • πŸ‡ΊπŸ‡ΈUnited States trackleft2 Tucson, AZ πŸ‡ΊπŸ‡Έ

    I am unable to reproduce, but in theory the error is occurring because the parameter passed to getVersionIdentifier is null instead of a string.

    protected function getVersionIdentifier(string $type): ?string {
    Should be updated to allow a null value like
    protected function getVersionIdentifier(?string $type): ?string {

    https://git.drupalcode.org/project/environment_indicator/-/blob/4.x/src/...

  • Pipeline finished with Success
    6 days ago
    Total: 150s
    #370228
  • πŸ‡ΊπŸ‡ΈUnited States trackleft2 Tucson, AZ πŸ‡ΊπŸ‡Έ
  • πŸ‡³πŸ‡±Netherlands helmo

    Both !81 and !82 solve the issue for me.

  • πŸ‡³πŸ‡ΏNew Zealand atowl

    Hi @isholgueras,

    We're upgrading from 4.0.19 -> 4.0.20.

    Appending to the reproduction, we noticed it during a drush deploy.

    I'm not a fan of changing the getVersionIdentifier parameter to ?string, because it should never be null i don't believe.

  • πŸ‡ΊπŸ‡¦Ukraine andriic

    Upgrading from 4.0.19 => 4.0.20.
    Missing configs values in environment_indicator.settings.yml leads to this error.

    This could happen if you didn't export configs after module update.
    Adding manually to the file and re-import configs fix the issue.

    version_identifier: 'environment_indicator_current_release'
    version_identifier_fallback: 'deployment_identifier'
  • πŸ‡ͺπŸ‡ΈSpain isholgueras

    I was able to reproduce the issue by running the following command to delete the config:

    drush ev "
    \$config = \Drupal::configFactory()->getEditable('environment_indicator.settings');
    \$config->clear('version_identifier')->save();
    "
    

    And this happens because the module was updated but the environment_indicator_update_8101 didn't run.

    I've created a new MR and I prefer to set a default value in case the config is not set. It seems that the problem is that the hook_update_N is not working well, but I think this will fix the issue safely.

    By default I've added the same values we have in the environment_indicator.settings.yml

  • πŸ‡ΊπŸ‡ΈUnited States joegraduate Arizona, USA

    MR !82 seems like the correct fix to me. +1 for RTBC on that MR.

  • πŸ‡ΊπŸ‡ΈUnited States trackleft2 Tucson, AZ πŸ‡ΊπŸ‡Έ

    Both !82 and !83 look good to me, I've tested both, and both work fine.

    It is concerning that the update isn't working as expected, we might consider moving it to an environment_indicator.post_update.php file.

    /**
     * @file
     * Post update functions for the Environment Indicator module.
     */
    
    /**
     * Set the version_identifier and version_identifier_fallback settings.
     */
    function environment_indicator_post_update_set_version_identifier_information() {
      $config_factory = \Drupal::configFactory();
      $settings = $config_factory->getEditable('environment_indicator.settings');
    
      // Check if the version_identifier setting exists, and set a default value if it does not.
      if ($settings->get('version_identifier') === NULL) {
        $settings->set('version_identifier', 'environment_indicator_current_release');
      }
    
      // Initialize version_identifier_fallback with the old default fallback value.
      if ($settings->get('version_identifier_fallback') === NULL) {
        $settings->set('version_identifier_fallback', 'deployment_identifier');
      }
    
      $settings->save();
    }
    
  • Pipeline finished with Skipped
    5 days ago
    #370920
  • πŸ‡ͺπŸ‡ΈSpain isholgueras

    It's strange, because if I run:

    ddev drush ev "
    \$config = \Drupal::configFactory()->getEditable('environment_indicator.settings');
    \$config->clear('version_identifier')->save();
    "
    

    and

    ddev drush ev "\Drupal::keyValue('system.schema')->set('environment_indicator', 8000);"
    

    in branch 4.x, the site breaks. After I run drush updb all works.

    If I run both commands in the branch of MR83, the site works well, and if I run drush updb the update prompts well, runs and everything works well.

    I'm going to merge MR83 and release it in the 4.0.21.

    Thanks all for the work and feedback. Much appreciated!

Production build 0.71.5 2024