Core requirements for 3.1.x: require D11.3+, PHP 8.3+

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.

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

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.

I rebased on the current 3.x and resolved a merge conflict from #3144002: Field permissions access override.

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:

#3224955: Allow themes to alter inline entity forms 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 #3413044: Get PHPStan level 1 passing and enable automated test job, which already adds a BC layer for the MigrationHelper constructor.

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

#3149783: Remove hardcoded word 'entities' in EntityInlineForm::getEntityTypeLabels()
#3413645: InlineEntityFormBase::getEntityTypeLabels() has the wrong docblock
#3413653: Deprecate InlineFormInterface::getEntityTypeLabels()

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...

@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 #2576445: Inline Entity Form stable release plan. 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.

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.

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.

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.

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.

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.

@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.

Re: #13:

we can no longer test against those versions now that Drupal CI is gone.

This is totally false. See https://www.drupal.org/project/views_taxonomy_term_name_into_id as a counter example. That's a contrib I maintain that supports D8 through D11. It's testing on all 4 core major versions in its GitLab CI pipelines, e.g.:

https://git.drupalcode.org/project/views_taxonomy_term_name_into_id/-/pi...

It's absolutely possible to support older versions, and test them.

Whether we want to do that here in IEF is another question. 😅 I'm still torn. But such a decision should be based on true considerations like technical debt vs. ease of use for sites that rely on this module, not bogus claims that it's impossible to have automated tests with older versions of core.

D8 and 9 aren't supported and haven't received any of the recent security updates. I don't think 3.x needs to keep that support, since the 8.x-1.x branch supports them. Having every branch support every version of Drupal doesn't make sense. Newer branches should drop support for older versions, in order to keep the codebase clean.

Per #2576445-73: Inline Entity Form stable release plan I think we should just ship 3.0.0 as-is, including with the older version support. We can bump our minimum dependencies and cleanup all the code for a 3.1.x or 4.0.x series as appropriate, but let's stop delaying the stable release.

Agreed.

Okay, great. Postponed until 3.0.x branch is open so 3.x can be for all the new slickness.

Since 3.0.x will support all of D11 and can be a "bridge release" for all earlier versions, Perhaps 3.1.x can target D11.3+ and D12. That's a pretty nice API cut-off, since it means OOP hooks, attributes for everything, PHP 8.5, etc. That would enable substantial cleanup.

Closed the old, outdated MR. Opened a new branch / MR for the current plan. D11.3 supports PHP 8.3, so for now, that's the minimum. I setup the GitLab CI jobs so "current" is 11.3.x and PHP 8.3, while "next minor" is 11.4.x and PHP 8.5.

3.x branch now requires 11.3.x and PHP 8.3. #3585018: Drupal 12 support is ready to proceed, along with other deprecation removals, conversion to OOP hooks, etc. See #3593827: 3.1.x / 4.0.x roadmap for more. Thanks, everyone!

Thanks for closing out these old issues!

One nit:

Perhaps 3.1.x can target D11.3+ and D12. That's a pretty nice API cut-off, since it means OOP hooks, attributes for everything, PHP 8.5, etc. That would enable substantial cleanup.

Drupal 12 requires PHP 8.5+, but Drupal 11.3+ requires PHP 8.3+. But it mostly does not matter because there are polyfills.

Re: #31: You're welcome! Thank you. #21 is outdated, no need to nit it. 😉 See #26, #29, and the issue title.

There is no need for php: 8.3 when Drupal 11 is required because that is the minimum for Drupal 11 anyway.

@liam morland: Yeah, I know it's not technically needed. I thought it was helpful to be explicit for my/our own clarity. I almost got it wrong, then realized my mistake, so I wanted to leave it an explicit statement/decision, not something we simply inherit from our core dependency.