Problem/Motivation

\Drupal\update\ModuleVersion::createFromSupportBranch() and \Drupal\update\ModuleVersion::createFromVersionString() both directly throw exceptions but do not document them with @throws tags.

Proposed resolution

Document them.

Remaining tasks

Do it

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#5 3212005-5.patch930 bytesguilhermevp
#2 3212005-2.patch822 bytesguilhermevp

Issue fork drupal-3212005

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

tedbow created an issue. See original summary.

guilhermevp’s picture

Status: Active » Needs review
StatusFileSize
new822 bytes

Sending patch. Please review.

tedbow’s picture

Status: Needs review » Needs work

@guilhermevp thanks for the patch!

+++ b/core/modules/update/src/ModuleVersion.php
@@ -43,6 +43,8 @@
+   * @throws \UnexpectedValueException

we need comments for why the exception is thrown. You can get an idea from the exception message and search in core for example messsages that go with @throws

guilhermevp’s picture

Assigned: Unassigned » guilhermevp

Thanks for the feedback! I will do it!

guilhermevp’s picture

Status: Needs work » Needs review
StatusFileSize
new930 bytes

Sending new patch, please review.

guilhermevp’s picture

Assigned: guilhermevp » Unassigned
tedbow’s picture

Status: Needs review » Needs work

@guilhermevp thanks for updating the patch. Here are a couple suggestions to make it extra clear to developers who get these exceptions what the problem

The only reason I know these reasons is because I helped add this class(and forgot to add the descriptions at the time 😞)

  1. +++ b/core/modules/update/src/ModuleVersion.php
    @@ -43,6 +43,9 @@
    +   *   Thrown when version string has unexpected core prefix.
    

    I think we should add some more context.

    how about
    Thrown when a legacy version string has a core prefix other than "8.x-". For example, version strings such as "7.x-1.0" are not supported.

  2. +++ b/core/modules/update/src/ModuleVersion.php
    @@ -100,6 +103,9 @@ private function __construct($major_version, $version_extra) {
    +   *   Thrown when branch is not supported.
    

    I think we should change this because a "supported branch" in Drupal has special meaning.

    For instance 8.9.x is a supported and 8.7.x is not supported currently.

    But the exception here is because the parameter $branch is an invalid string that does not end with "."

    So maybe
    Thrown when $branch invalid because it does not end in "."

guilhermevp’s picture

Assigned: Unassigned » guilhermevp

Working on it!

guilhermevp’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

One small grammatical fix and then I think this is RTBC!

guilhermevp’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

Ah, sorry, I should have maybe been clearer in my review...!

guilhermevp’s picture

Status: Needs work » Needs review

Thanks @phenaproxima! Sometimes my english just breaks.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Boom! Looks great.

tedbow’s picture

Assigned: guilhermevp » Unassigned

Thanks @guilhermevp and @phenaproxima!!!!

  • catch committed ba7c2c5 on 9.3.x
    Issue #3212005 by guilhermevp, tedbow, phenaproxima: Add @throws docs to...
catch’s picture

Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed ba7c2c5 and pushed to 9.3.x. Thanks! Also cherry-picked to 9.2.x

  • catch committed dd32dcd on 9.2.x
    Issue #3212005 by guilhermevp, tedbow, phenaproxima: Add @throws docs to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.