Failsafe conversion of block_classes_stored

Created on 11 August 2024, about 2 months ago
Updated 27 August 2024, about 1 month ago

Problem/Motivation

In ✨ Storing block_classes_stored as a string value leads to poor developer experience Fixed , an update script was introduced that converts the block_classes_stored setting from a JSON string to an array.
The problem is that it does not check whether that setting has already been converted*, resulting in a JSON error because json_decode expects a string, not an array.
The update script will fail forever.

*Cases where this can happen: we're running the update script in a development environment, export the config changes to the filesystem, sync these files to the production environment, and import them. When running drush updb afterwards, the conversion has already taken place, and the update fails.

Proposed resolution

Do a simple is_string() check before converting the data.

Remaining tasks

πŸ› Bug report
Status

RTBC

Version

2.0

Component

Code

Created by

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

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

Merge Requests

Comments & Activities

  • Issue created by @mrshowerman
  • Pipeline finished with Failed
    about 2 months ago
    Total: 174s
    #250825
  • Pipeline finished with Success
    about 2 months ago
    Total: 162s
    #251721
  • πŸ‡§πŸ‡·Brazil renatog Campinas

    Nice catch. Thanks for the MR

    Added thread above there

  • Pipeline finished with Failed
    about 2 months ago
    Total: 144s
    #252035
  • Pipeline finished with Success
    about 2 months ago
    Total: 176s
    #252037
  • Issue was unassigned.
  • Status changed to Needs review about 2 months ago
  • πŸ‡©πŸ‡ͺGermany mrshowerman Munich

    Addressed feedback on the MR

  • πŸ‡©πŸ‡ͺGermany szeidler Berlin

    Just a note to the initial issue description: it is quite risky to run drush config:import before drush up.

    If you deploy changes the best practice order is https://www.drush.org/12.x/deploycommand/ or the corresponding documented individual commands.

  • πŸ‡«πŸ‡·France dydave

    Thanks a lot Stephan (@szeidler) for the very helpful feedback!

    Unrelated with this ticket, but just by curiosity:
    To deploy/build a project, do you usually like using the drush deploy command or rather each one of the commands separately?

    drush updb
    drush cr
    drush cim
    drush cr
    

    along with hook_install

    I've seen projects use a lot hook_deploy, along with drush deploy, but personally, I'm not a big fan...

    Would be glad to hear if you have any thoughts on that.
    Thanks in advance!

  • πŸ‡©πŸ‡ͺGermany mrshowerman Munich

    Thanks @szeidler for making this clear.
    We have a typical scenario where we need to migrate fields from one type to another (e.g. plaintext to formatted text), so we first import configuration to add the new field, then run updates to copy the field values over through HOOK_update_N(). Doing it the other way around would not work, since the new field does not exist when the hook is run.
    Wasn't aware of HOOK_deploy_NAME(), which seems to improve things a lot. Will look deeper into it.

    Nevertheless, I still think that update script should not fail if for whatever reason the setting has already been updated.

  • Status changed to RTBC about 1 month ago
  • πŸ‡³πŸ‡ΏNew Zealand stewest Wellington

    This patch works to now now allow database updates to run

    block_class 20017 hook_update_n 20017 - Convert block_classes_stored from JSON to a sequence.

    https://git.drupalcode.org/project/block_class/-/merge_requests/52.diff

  • πŸ‡ΊπŸ‡ΈUnited States rkelbel48

    I can also confirm the patch works as needed, database updates run just fine now.

Production build 0.71.5 2024