Problem/Motivation

We can modernize the codebase a bit now that Drupal and PHP have evolved. E.g., NodeSelectTerms.php can use constructor property promotion, return types, typed arguments, etc. We also need some automated tests.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork bulk_edit_terms-3532103

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

bkosborne created an issue. See original summary.

bkosborne opened merge request !8

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Status: Active » Needs review

Okay, MR added and ready for review.

fjgarlin’s picture

fjgarlin commented

Thanks for this. I will review it shortly. If this goes through, I’m thinking of creating a 2.x branch as we’re dropping support for older versions and adding support for newer ones.

I haven’t checked the code in detail yet, but if we require a minimum version of PHP to leverage the new language features we should add that requirement to the composer file too.

[edit] I guess that the PHP version might be determined by the minimum Drupal supported version as we don’t have a composer file.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

Yea, I think we're good at just requiring Drupal 10, which requires PHP 8.1 already. I don't think I used any language features beyond 8.1.

fjgarlin’s picture

fjgarlin commented
Version: 8.x-1.x-dev » 2.x-dev

I just created the 2.x branch and moved the target branch of the MR to be that. I left some feedback on the MR, really minimal, and answer the question in the `@todo`. Feel free to do something about it here or in a follow-up.

The code looks really good and it'd be ready to merge into a 2.x branch.

I'll let you see the feedback first before marking it RTBC. Great job!

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Version: 2.x-dev » 8.x-1.x-dev

Thanks for the review! I addressed the feedback and pushed.

fjgarlin’s picture

fjgarlin commented
Version: 8.x-1.x-dev » 2.x-dev
fjgarlin’s picture

fjgarlin commented
Status: Needs review » Needs work

Small error in "phpcs". Once fixed, it can go directly into RTBC.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Status: Needs work » Reviewed & tested by the community

Fixed! Once this is merged, I can start work on the other two issues I created.

  • fjgarlin committed 0d01c793 on 2.x authored by bkosborne 
    Issue #3532103 by bkosborne, fjgarlin: Modernize codebase & add tests
fjgarlin’s picture

fjgarlin commented
Status: Reviewed & tested by the community » Fixed

The MR is merged now. Thanks a lot for the improvements so far! Looking forward to reviewing more issues.

Status: Fixed » Closed (fixed)

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