Fix soft BC break with Uuid::fromString() signature
#383
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Uses an assertion to indicate the `$uuid` variable cannot be an empty string, satisfying docblock on `LazyUuidFromString`.
Jamesking56
approved these changes
Aug 9, 2021
Codecov Report
@@ Coverage Diff @@
## main #383 +/- ##
=========================================
Coverage 96.91% 96.91%
Complexity 533 533
=========================================
Files 64 64
Lines 1522 1522
=========================================
Hits 1475 1475
Misses 47 47
|
|
Thanks for catching this, and thank you for contributing! 🎉 |
aurelien-reeves
added a commit
to cucumber/common
that referenced
this pull request
Jul 4, 2022
The following fix is needed: ramsey/uuid#383
ciaranmcnulty
pushed a commit
to cucumber/common
that referenced
this pull request
Jul 4, 2022
The following fix is needed: ramsey/uuid#383
aurelien-reeves
added a commit
to cucumber/common
that referenced
this pull request
Jul 4, 2022
* Check lowest versions of dependencies This is mostly because of #2034 - we need to check that everything still works with the 'lowest' versions of the dependencies There aren't any external dependencies yet so currently this will only detect when Gherkin doesn't work with Messages any more, or when we've written tests that don't work with older versions of the test tools * Fix COMPOSER_FLAGS var in Makefile * Switch install to update as we gitignore composer.lock * Attempt to fix php 'bc' builds * Update ramsey/uuid to minimum 4.2.1 The following fix is needed: ramsey/uuid#383 * Explicitly depend on high version of nikic/php-parser This is used in static analysis and older versions can't deal with enums directly Becuase we don't depend on it directly Psalm was allowing an older version to install * Silence a static analysis error in tests This is something we still want to check for even if the types say it's redundant - JSON decoding could be throwing an error here * Remove path repository for low-dependency build Co-authored-by: aurelien-reeves <aurelien.reeves@smartbear.com>
cukebot
pushed a commit
to cucumber/gherkin-php
that referenced
this pull request
Jul 4, 2022
* Check lowest versions of dependencies This is mostly because of cucumber/common#2034 - we need to check that everything still works with the 'lowest' versions of the dependencies There aren't any external dependencies yet so currently this will only detect when Gherkin doesn't work with Messages any more, or when we've written tests that don't work with older versions of the test tools * Fix COMPOSER_FLAGS var in Makefile * Switch install to update as we gitignore composer.lock * Attempt to fix php 'bc' builds * Update ramsey/uuid to minimum 4.2.1 The following fix is needed: ramsey/uuid#383 * Explicitly depend on high version of nikic/php-parser This is used in static analysis and older versions can't deal with enums directly Becuase we don't depend on it directly Psalm was allowing an older version to install * Silence a static analysis error in tests This is something we still want to check for even if the types say it's redundant - JSON decoding could be throwing an error here * Remove path repository for low-dependency build Co-authored-by: aurelien-reeves <aurelien.reeves@smartbear.com>
cukebot
pushed a commit
to cucumber/messages-php
that referenced
this pull request
Jul 4, 2022
* Check lowest versions of dependencies This is mostly because of cucumber/common#2034 - we need to check that everything still works with the 'lowest' versions of the dependencies There aren't any external dependencies yet so currently this will only detect when Gherkin doesn't work with Messages any more, or when we've written tests that don't work with older versions of the test tools * Fix COMPOSER_FLAGS var in Makefile * Switch install to update as we gitignore composer.lock * Attempt to fix php 'bc' builds * Update ramsey/uuid to minimum 4.2.1 The following fix is needed: ramsey/uuid#383 * Explicitly depend on high version of nikic/php-parser This is used in static analysis and older versions can't deal with enums directly Becuase we don't depend on it directly Psalm was allowing an older version to install * Silence a static analysis error in tests This is something we still want to check for even if the types say it's redundant - JSON decoding could be throwing an error here * Remove path repository for low-dependency build Co-authored-by: aurelien-reeves <aurelien.reeves@smartbear.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
$uuidparameter onUuid::fromString()was changed fromstringtonon-empty-stringthis breaks BC for people using static analysis in their downstream projects.Description
I've reverted the change to the docblock and used an assertion to teach static analysis tools when the
$uuidvariable is a non empty string instead.Motivation and context
Whilst this isn't a language level backwards compatibility break, it does break when consumers are using static analysis tools on their own code base because they'd need to add checks to test if the input is non empty before calling
Uuid::fromString().How has this been tested?
I've added a unit test covering the call to
Uuid::fromString('').Types of changes
PR checklist