Skip to content
Snippets Groups Projects

Improve error reporting on content mismatch

Open Improve error reporting on content mismatch
issue/drupal-3525769:3525769-improve-error-reporting into 11.x
3 unresolved threads
Open Dezső Biczó requested to merge issue/drupal-3525769:3525769-improve-error-reporting into 11.x
3 unresolved threads

Closes #3525769

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
composer/Plugin/RecipeUnpack/RootComposer.php
139 140
140 141 $this->io->write("Unpacking has updated the root composer files.", verbosity: IOInterface::VERBOSE);
141 142
142 assert(self::checkRootPackage($composer_content, $this->composer->getPackage()), 'Composer root package and composer.json match');
  • Dezső Biczó
    Dezső Biczó @mxr576
    Author

    Should this be an exception instead? :thinking:

  • A
    Alex Pott @alexpott
    Maintainer

    No an assert is best here. This is mostly about ensuring in test and development environments.

  • Dezső Biczó
    Dezső Biczó @mxr576
    Author

    In which cases this code runs in production environments? When it is called by Project Browser? If that happens, should the process fail and let the user know that the unpack process finished with an invalid state and the root composer.json is not up to date?

    Based on my testing, whenever this assert fails, every Composer command afterwards starts acting differently because Composer tries to resolve the problem.

  • A
    Alex Pott @alexpott
    Maintainer

    As core.api.php says:

     * There are two php settings which affect runtime assertions. The first,
 * assert.exception, should always be set to 1. The second is zend.assertions.
 * Set this to -1 in production and 1 in development.
  • Dezső Biczó
    Dezső Biczó @mxr576
    Author

    Well, what I actually asked is in what conditions the Recipe Unpack plugin will run in a production environment? Project Browser is the only thing that occurs to me that could run this plugin when it will support install Recipes on the UI... otherwise, I would assume this code only runs in development environment, which case assert() is fine.

    However, if unpack will run in production envs than throwing and handling an exception could be a better alternative.

    Anyway, this thread is not strictly related to the change that I proposed, considering to switch from assert() to exception could be a different issue.

  • Please register or sign in to reply
  • Dezső Biczó added 1 commit

    added 1 commit

    • 26d6eae4 - Improve error reporting on content mismatch

    Compare with previous version

  • Alex Pott @alexpott started a thread on an old version of the diff
    • composer/Plugin/RecipeUnpack/RootComposer.php
    139 140
    140 141 $this->io->write("Unpacking has updated the root composer files.", verbosity: IOInterface::VERBOSE);
    141 142
    142 assert(self::checkRootPackage($composer_content, $this->composer->getPackage()), 'Composer root package and composer.json match');
    143 $diff = self::compareComposerContentWithRootPackage($composer_content, $this->composer->getPackage());
    144 assert(empty($diff),
    145 sprintf("Composer root package and composer.json mismatch detected.\nrequire: %s\nrequire-dev: %s\n",
    146 array_key_exists('require', $diff) ? implode(', ', $diff['require']) : 'OK',
    147 array_key_exists('require-dev', $diff) ? implode(', ', $diff['require-dev']) : 'OK',
    148 )
    149 );
  • Dezső Biczó added 2 commits

    added 2 commits

    • a517263b - Only run diff when assertions are enabled
    • 1c1ffcb0 - Only generate diff when assertions are active

    Compare with previous version

  • T
    Tugboat.qa @tugboat
    Guest

    Tugboat has finished building the preview for this merge request!

    Link:

    Dashboard:

  • Dezső Biczó
    Dezső Biczó @mxr576 started a thread on the diff
    • composer/Plugin/RecipeUnpack/RootComposer.php
    139 140
    140 141 $this->io->write("Unpacking has updated the root composer files.", verbosity: IOInterface::VERBOSE);
    141 142
    142 assert(self::checkRootPackage($composer_content, $this->composer->getPackage()), 'Composer root package and composer.json match');
    143 if (ini_get('zend.assertions') > 0) {
    Please register or sign in to reply