The new key is necessary to make it explicit that this module supports Drupal 9.

There's also an entity.manager service usage in \Drupal\eu_cookie_compliance\Form\EuCookieComplianceConfigForm::create() that drupal-check doesn't find.

I also removed some keys in composer.json that only work on project-level and not in a module.

CommentFileSizeAuthor
#21 3104120-21-interdiff.txt614 bytesberdir
#21 3104120-21.patch3.52 KBberdir
#20 3104120-20.patch2.92 KBberdir
#19 3104120-19.patch2.89 KBberdir
#17 3104120_17.patch1.06 KBberdir
#16 3104120_16.patch2.05 KBshimpy
#14 3104120_11.patch2.47 KBshimpy
#12 3104120_11.patch1.85 KBshimpy
#9 3104120-7.patch3.43 KBankush_03
#6 3104120-6.patch3.07 KBankush_03
#4 3104120-4.patch2.38 KBmbovan
#4 3104120-4-1-interdiff.txt589 bytesmbovan
eu_cookie_compliance-d9.patch2.43 KBberdir

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: +Drupal 9 compatibility
mbovan’s picture

mbovan commented
Status: Needs review » Needs work
Issue tags: +ContributionWeekend2020, +ContributionWeekendCH 
  1. +++ b/composer.json
@@ -3,7 +3,9 @@
+  "require": {
+    "drupal/core": "^8 || ^9"
+  },

    This should not be needed anymore. 

  2. +++ b/composer.json
@@ -33,11 +35,6 @@
+  }
+

    An extra line in composer.json.

mbovan’s picture

mbovan commented
Status: Needs work » Needs review
StatusFileSize
new 3104120-4.patch2.38 KB
new 3104120-4-1-interdiff.txt589 bytes

Addressed #3.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

True, I suppose we could completely skip the composer.json completely then now, that cleanup is even more unrelated now ;)

ankush_03’s picture

ankush_03
Location Gurgaon, India 🇮🇳
commented
StatusFileSize
new 3104120-6.patch3.07 KB

Added new patch please review !

ankush_03’s picture

ankush_03
Location Gurgaon, India 🇮🇳
commented
Issue tags: +GCWIndia2020
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

That is not correct, that has additional wrong changes and re-adds the composer.json change.

ankush_03’s picture

ankush_03
Location Gurgaon, India 🇮🇳
commented
StatusFileSize
new 3104120-7.patch3.43 KB

Changes added.

abhisekmazumdar’s picture

abhisekmazumdar
he/him
Primary language English
Location India
commented
Status: Needs work » Needs review
mbovan’s picture

mbovan commented
Status: Needs review » Needs work 
  1. index 8a359a8..d2a34ee 100644
--- a/composer.json

--- a/composer.json
+++ b/composer.json

+++ b/composer.json
@@ -3,7 +3,6 @@

@@ -3,7 +3,6 @@
   "description": "This module aims at making the website compliant with the new EU cookie regulation.",
   "type": "drupal-module",
   "license": "GPL-2.0+",
-  "minimum-stability": "dev",
   "homepage": "https://drupal.org/project/eu_cookie_compliance",
   "keywords": [
     "Drupal",
@@ -34,10 +33,7 @@

@@ -34,10 +33,7 @@
     "irc": "irc://irc.freenode.org/drupal-contribute",
     "source": "https://cgit.drupalcode.org/eu-cookie-compliance"
   },
-  "repositories": [
-    {
-      "type": "composer",
-      "url":  "https://packages.drupal.org/8"
-    }
-  ]
+  "require": {
+    "drupal/core": "^8 || ^9"
+  }
 }

    This all can be removed now. 

  2. +++ b/src/Form/EuCookieComplianceConfigForm.php
@@ -47,7 +48,14 @@ class EuCookieComplianceConfigForm extends ConfigFormBase {
-
+  ¶
+  /**
+   * Drupal\Core\Entity\EntityTypeManagerInterface definition.
...
+   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
+   */
+  protected $entityTypeManager;
+  ¶

@@ -62,13 +70,14 @@ class EuCookieComplianceConfigForm extends ConfigFormBase {
-  public function __construct(ConfigFactoryInterface $config_factory, PathValidatorInterface $path_validator, RequestContext $request_context, RoleStorageInterface $role_storage, ModuleHandlerInterface $module_handler) {
+  public function __construct(ConfigFactoryInterface $config_factory, PathValidatorInterface $path_validator, RequestContext $request_context, RoleStorageInterface $role_storage, ModuleHandlerInterface $module_handler, EntityTypeManagerInterface $entity_type_manager) {
...
+    $this->entityTypeManager = $entity_type_manager;

    This should not be needed since we are not using entity type manager in this class.

shimpy’s picture

shimpy
Location India
commented
Status: Needs work » Needs review
StatusFileSize
new 3104120_11.patch1.85 KB

I have uploaded a patch with few changes. Review.

mbovan’s picture

mbovan commented
Status: Needs review » Needs work

We still need replacement for entity.manager to entity_type.manager introduced in #1.

shimpy’s picture

shimpy
Location India
commented
Status: Needs work » Needs review
StatusFileSize
new 3104120_11.patch2.47 KB

I have corrected the entity.manager to entity_type.manager.
and other issues well. Hope this will work.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Now you removed the composer.json completely. You should just remove the changes to it, so it doesn't exist in the patch, but remains as-is in the module.

shimpy’s picture

shimpy
Location India
commented
Status: Needs work » Needs review
StatusFileSize
new 3104120_16.patch2.05 KB

Thanks all for reviewing my patches. Giving it one more try.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new 3104120_17.patch1.06 KB

Now we have some changes in the file again, what I mean is this patch.

To get that, I applied your patch, then git checkout HEAD composer.json and then created a new patch.

mbovan’s picture

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

I think we are good with #17.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 3104120-19.patch2.89 KB

Turns out there is at least one deprecation hiding in there, and it's one that requires 8.8. Drupal::service() calls is something that drupal-check currently doesn't find. That's where test would be handy ;)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new 3104120-20.patch2.92 KB

And that means we need to remove the core: 8.x key completely to ensure older versions aren't compatible anymore.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new 3104120-21.patch3.52 KB
new 3104120-21-interdiff.txt614 bytes

One more thing, apparently the arguments for replace() in twig have changed to an array, but it is anyway much easier to use the clean_css filter that is provided by core.

svenryen’s picture

svenryen commented

Thanks for fixing this. I'll take a look and review this week.

svenryen’s picture

svenryen commented 
+++ b/eu_cookie_compliance.info.yml
@@ -2,7 +2,7 @@ name: 'EU Cookie Compliance'
-core: 8.x
+core_version_requirement: ^8.8 || ^9

@berdir Won't this change make it impossible to use the module with Drupal versions below 8.8? What's the general advise on this? Should we bump the major version to indicate it's a change that breaks backwards compatibility?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes, it will.

It's up to you when to commit that, I've committed patches that require 8.8 now to token, pathauto, redirect, paragraphs and more, so at least 4 of the top 10 modules now require 8.8 already. I didn't do releases yet, that would still allow me to easily do a security release on top of the previous release.

And no, requiring a min version of a dependency is not a BC break per semver, that's a common and ongoing process that will continue over the next years. Eventually you're require 9.2 or something for some new feature that you want to rely on.

You could do that if you want to, but I'd advise against it at least for now, as semver for contrib should be enabled in the next weeks on d.o, meaning you could do 2.x.y instead of 8.x-2.x. But I still think this isn't a reason to do that, only if you want to break your own API's.

  • svenryen committed c5a18b6 on 8.x-1.x authored by Berdir 
    Issue #3104120 by Berdir, shimpy, ankushgautam76@gmail.com, mbovan:...
svenryen’s picture

svenryen commented
Status: Needs review » Fixed

Thanks for clearing that up. WIth D8.7 being unsupported I think we can surely push this fix. Thanks for the through work all!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

One clarification. 8.7 still gets security updates until 8.9/9.0 gets released, but there's no rule or even guideline on how long contrib maintainers still have to support it with their modules. But IMHO, it's perfectly fine to drop support for that now, I think it's fair to expect that people need to update core if they want to update their contrib modules to the latest version as well, and composer will ensure that it does not install an incompatible version and people doing it manually with an old core version will get an error in Drupal.

The only awkward situation is if you would need to release a security update, then it would be nice if users that are still on a core version with security update could update as well, that's why I'm waiting a bit with doing new releases atm. But even then, there's no rule right now that a contrib security update needs to support all core versions that still have security support. This will be easier with semver, then we can do a 2.1 release and later on a 2.0.1 and 2.1.1 security update if we have to.

  • a3ad136 committed on 8.x-1.x 
    Merge remote-tracking branch 'origin/8.x-1.x' into 8.x-1.x

* origin/8.x...

Status: Fixed » Closed (fixed)

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

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Thanks for all the great work. Opened #3131515: Create Drupal 9 compatible release of EU Cookie Compliance.