Drop D9 and PHP7 Support before stable

Created on 8 January 2024, 12 months ago

Problem/Motivation

Drop EOL version support before stable.

Proposed resolution

Do it.

πŸ“Œ Task
Status

Active

Version

3.0

Component

Code

Created by

πŸ‡©πŸ‡ͺGermany geek-merlin Freiburg, Germany

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @geek-merlin
  • πŸ‡ΊπŸ‡ΈUnited States dcam

    Is there any objection to removing the drupal/core requirement from the composer.json file when we do this? The D.o documentation says it's better to not have it in there, see https://www.drupal.org/docs/develop/using-composer/add-a-composerjson-fi... β†’ . I don't know if there is a reason to have it in there or if this was done because someone thought it's appropriate. I know that I mistakenly did that once upon a time. Note that if there is an incompatibility with a certain version of Core, then I find it's much better to declare a conflict in the composer.json file instead of a requirement.

    And on a personal note: every time I try to update a module with its dependencies (including IEF) and Composer proceeds to download a few dozen Core dependencies I tell myself that I'm going to go on a crusade to fix this everywhere.

  • πŸ‡ΊπŸ‡ΈUnited States dww

    +1 to #2 and removing that requirement from composer.json.

  • Merge request !103Drop D9 and PHP7 Support before stable β†’ (Open) created by dcam
  • Pipeline finished with Success
    12 months ago
    Total: 508s
    #73959
  • Status changed to Needs review 12 months ago
  • πŸ‡ΊπŸ‡ΈUnited States dcam

    I just did a whole lot of deleting. I figured that we can let the Core PHP requirement take priority, which is version 8.1.

    Admittedly I'm interested in getting this in so I can use constructor property promotion in the PHPStan issue.

  • Status changed to Needs work 11 months ago
  • πŸ‡©πŸ‡ͺGermany geek-merlin Freiburg, Germany

    According to #3407407-6: Clarify how to site-upgrade IEF 2.x => 1.x|3.x β†’ : "Please don't drop D9".
    I checked the core usage statistics and yes, there are still many installations on d9, more than i'd have guessed.
    Also, dropping D9 currently does not bring us much imho.

    I don't see problems with php8.1 though. Even my super conservative centos hosters support it.
    Comment #3407407-8: Clarify how to site-upgrade IEF 2.x => 1.x|3.x β†’ also states this.
    And php8.1 hels us clean up code.

    Updating IS accordingly and setting NW.

  • First commit to issue fork.
  • Pipeline finished with Success
    11 months ago
    Total: 558s
    #87578
  • Status changed to Needs review 11 months ago
  • πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

    I rebased on the current 3.x and resolved a merge conflict from πŸ› Field permissions access override Fixed .

    I restored compatibility with Drupal 9, following the decision in Comment #6. But then Comment #5 no longer applies, so I added a minimum PHP version in the .info.yml file (but not in composer.json).

    I also reviewed the changes made for compatibility with Drupal 10. I did not find any changes that would make this module incompatible with Drupal 9. I did find two things that require further work:

    ✨ Allow themes to alter inline entity forms Fixed added a parameter to the EntityInlineForm constructor without a BC layer.

    There is a comment in InlineFormInterface::getEntityLabels():

    * @todo Remove when #1850080 lands and IEF starts requiring Drupal 8.1.x

    Since #1850080: Entity type labels lack plurality, cannot generate UI text based on label if plural is needed β†’ was fixed a long time ago, we should get rid of that method instead of updating it.

    I am not sure whether to fix those problems as part of this issue or create new issues. We might fix the first one as part of πŸ“Œ Get PHPStan level 1 passing and enable automated test job Active , which already adds a BC layer for the MigrationHelper constructor.

  • πŸ‡ΊπŸ‡ΈUnited States dww

    Thanks! All the getEntityLabels() cleanup stuff already have dedicated issues. We don't want to change that here, nor open any new issues:

    πŸ“Œ Remove hardcoded word 'entities' in EntityInlineForm::getEntityTypeLabels() Fixed
    πŸ“Œ InlineEntityFormBase::getEntityTypeLabels() has the wrong docblock Needs review
    πŸ“Œ Deprecate InlineFormInterface::getEntityTypeLabels() Needs review

    That said, I'm kind of torn here. If we're supporting D9 (+1 to that), seems weird to require a higher PHP than core did for 9.x. I'm reluctant to move forward with dropping PHP 7.4 support until we're ready to drop D9, as nice as that would be for us maintainers...

  • πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

    @dww

    I am happy to leave the decisions to the maintainers. I just want to make sure that you read the earlier comments on this issue about whether to keep or drop support for D9 and for PHP 7.

    Also, this is one of the issues listed as a "Must have" on 🌱 Inline Entity Form stable release plan Active . If this issue is postponed, then please update the plan issue.

    It is OK with me if you postpone this issue until IEF 4.0. It is also OK with me if you drop support for D9 and PHP 7 in the 3.x branch and keep it in the 1.x branch.

  • πŸ‡¨πŸ‡¦Canada mandclu

    Weighing in with my perspective on this. I personally think that IEF is a major usability win for content creators, and as such should be a strong contender for inclusion in Starshot. Unfortunately, not having a stable release for Drupal 10 or 11 means that these version have no security team coverage, which I would consider a strong argument against inclusion in Starshot. This issue is listed as a blocker to a stable release.

    I totally respect wanting to maintain backwards compatibility. That said, PHP 7.4 and Drupal 9 are both unsupported at this point. I think it would be valid to assert that sites running unsupported versions of both shouldn't expect to be able to run the latest release of all the modules they use. Those sites can continue to use older releases of IEF, even if newer (hopefully stable) releases are not compatible.

  • πŸ‡¨πŸ‡¦Canada Liam Morland Ontario, CA πŸ‡¨πŸ‡¦

    I agree with @mandclu. inline_entity_form 3.x should only support versions of Drupal that are supported when it reaches full release and perhaps only the latest version of Drupal. It should be set to ^10.2 || ^11 or perhaps ^10.3 || ^11. People using old Drupal or PHP can use old inline_entity_form too.

  • πŸ‡ΊπŸ‡ΈUnited States dcam

    Things have changed in the last year which I feel makes it necessary to drop the compatibility with unsupported versions of Drupal Core: we can no longer test against those versions now that Drupal CI is gone. With the switch to GitLab CI we're only testing against supported versions. Therefore, we can't make any guarantee of compatibility with old versions. They need to be dropped. I've done it with all of the other contrib modules that I maintain. I suggest that we do it here. It's pointless and potentially harmful to give the appearance of compatibility.

    As it's already been said: people using old versions of core can just continue to use old versions of IEF.

  • Pipeline finished with Success
    2 months ago
    Total: 469s
    #324718
  • πŸ‡ΊπŸ‡ΈUnited States dcam

    I updated the MR and dropped compatibility with unsupported versions of Core. I also removed the PHP requirement from the info.yml file. There's no reason to have it if we aren't using PHP functions from versions higher than what Core requires. Since we were previously requiring PHP 7.2 it isn't an issue.

  • πŸ‡¨πŸ‡¦Canada Liam Morland Ontario, CA πŸ‡¨πŸ‡¦

    You could keep the entry in composer.json and update it to match core_version_requirement. That would mean that people updating with Composer won't get a version that it too new.

  • πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

    @liam morland:

    When d.o provides package metadata for Composer, it inspects the .info.yml. Adding an entry for Drupal core to composer.json is redundant, and it would mean that you have to update the compatibility information in two places going forward.

    I cannot review this issue and mark it RTBC, since I worked on the MR. But the current MR is pretty simple:

    1. Remove requirements for php and drupal/core from composer.json.
    2. Update core_version_requirement to ^10.2 || ^11. That is, drop compatibility unsupported versions of Drupal core.
    3. Fix an unrelated typo in the .info.yml file.

    What we really need is for the active maintainers to agree on what to do. Personally, I agree with @dcam in #13.

Production build 0.71.5 2024