Problem/Motivation

Follow up from #3323990: Migrate Drupal reports wrong version of Drupal if pointed at a Drupal 9 or 10 database from @benjifisher

simplify getLegacyDrupalVersion() further. Copying my suggestions from there:

Put a lot less in the try block.
Return early instead of assigning to $version_string and then returning it at the end

Steps to reproduce

NA

Proposed resolution

From the quote*

  • Put a lot less in the try block.
  • Return early instead of assigning to $version_string and then returning it at the end

Remaining tasks

Implement
Review

User interface changes

NA

API changes

NA

Data model changes

NA

Release notes snippet

NA

Issue fork drupal-3437178

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:

About issue forks

Comments

smustgrave created an issue. See original summary.

immaculatexavier made their first commit to this issue’s fork.

immaculatexavier opened merge request !7264

immaculatexavier’s picture

immaculatexavier commented
Status: Active » Needs work

Created MR for the proposed resolution, But PHPUnit test fails.

quietone made their first commit to this issue’s fork.

quietone’s picture

quietone commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Forgot about this one.

But refactoring seems fine and follows what was proposed in the original issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

The new code seems to have duplicated a comment. Also I left a suggestion for further refactoring on the MR.

quietone’s picture

quietone commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Appears suggestion from #alexpott has been addressed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 4881479469 to 11.x and 6b2ecdbb73 to 10.3.x. Thanks!

  • alexpott committed 6b2ecdbb on 10.3.x 
    Issue #3437178 by immaculatexavier, quietone, smustgrave, alexpott:...

  • alexpott committed 48814794 on 11.x 
    Issue #3437178 by immaculatexavier, quietone, smustgrave, alexpott:...

Status: Fixed » Closed (fixed)

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