- 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.
- Status changed to Needs review
12 months ago 5:33am 9 January 2024 - πΊπΈ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 6:56pm 28 January 2024 - π©πͺ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.
- Status changed to Needs review
11 months ago 11:37pm 4 February 2024 - πΊπΈ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 incomposer.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 reviewThat 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 oldinline_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.
- πΊπΈ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 matchcore_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 tocomposer.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:
- Remove requirements for
php
anddrupal/core
fromcomposer.json
. - Update
core_version_requirement
to^10.2 || ^11
. That is, drop compatibility unsupported versions of Drupal core. - 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.
- Remove requirements for