This module depends upon the Encrypt module, as declared in the .info.yml file. As such, the dependency should also be included in composer.json, so that dependencies can be correctly managed. This means that if/when this module is removed from Composer, it will also remove the Encrypt module (if no other modules depend upon it). It allows for better code management of D8 sites.

I'll attach a patch to the first comment.

CommentFileSizeAuthor
#2 real_aes-d8-add_composer_dependency_on_encrypt_module-3065766-1.patch635 bytesjaypan

Comments

Jaypan created an issue. See original summary.

jaypan’s picture

jaypan
Location Victoria, BC
commented
Status: Active » Needs review
StatusFileSize
new real_aes-d8-add_composer_dependency_on_encrypt_module-3065766-1.patch635 bytes

Patch attached. Needs review. Note that I also updated the license identifier, as the one being used previously has been deprecated in favor of the one I changed it to. This can be confirmed by running composer validate.

rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented
Status: Needs review » Reviewed & tested by the community

  • rlhawk committed e0c2203 on 8.x-2.x authored by Jaypan 
    Issue #3065766 by Jaypan: Composer dependency should be added for...
rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented
Status: Reviewed & tested by the community » Fixed
jcnventura’s picture

jcnventura
Primary language English
commented

I wonder why this was necessary. Drupal manages module dependency transparently via the composer packagist. The dependency on encrypt in .info.yml will add a composer dependency on the encrypt module. What this change did was to make explicit what the drupal.org packagist was doing implicitly. It's not bad, but it was also unnecessary, unless something escapes me here.

I think the best illustration why this was unnecessary is the fact that we've been using composer to install real_aes for 5 years now, and only now someone decided to create this issue.

jaypan’s picture

jaypan
Location Victoria, BC
commented

I wonder why this was necessary. Drupal manages module dependency transparently via the composer packagist.

No it doesn't. This module declares a dependency on Real AES in the info file, but one was not declared in Composer. This meant that if you wanted Real AES, you would need to require both drupal/encrypt and drupal/real_aes. Without doing the first, the Real AES module cannot be enabled,

Composer should manage all dependencies. Therefore after requiring a package, you should not get an error about a missing package when trying to enable it. That means Composer is not managing the dependencies. This is what was happening before this patch. With this patch, Composer will now manage this module's dependencies.

I think the best illustration why this was unnecessary is the fact that we've been using composer to install real_aes for 5 years now, and only now someone decided to create this issue.

That's not a logical argument. I clearly ran into a problem which is why I opened a ticket, and create a patch. Using your logic, I think the best illustration of why this was necessary is that I did it, and included reasoning in the original post.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

The issue summary here and the subsequent conversation is lacking a set of steps to repeat the problem and observe what might be fixed by making this change. Here's what I did to try to understand this situation and what I observed.

I started by creating a fresh drupal project:

composer create-project drupal/recommended-project my_site_name_dir

This got me a working directory which I added to a local git repo and committed so I could start from a known good state.

To see the behavior "before" this patch I required real aes 2.2:

composer require 'drupal/real_aes:^2.2'

This seemed to install stuff just fine:

./composer.json has been updated
Running composer update drupal/real_aes
Loading composer repositories with package information
Updating dependencies
Lock file operations: 5 installs, 0 updates, 0 removals
  - Locking defuse/php-encryption (v2.3.1)
  - Locking drupal/encrypt (3.0.0)
  - Locking drupal/key (1.15.0)
  - Locking drupal/real_aes (2.4.0)
  - Locking paragonie/random_compat (v9.99.100)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 5 installs, 0 updates, 0 removals
  - Downloading drupal/key (1.15.0)
  - Downloading drupal/encrypt (3.0.0)
  - Downloading paragonie/random_compat (v9.99.100)
  - Downloading defuse/php-encryption (v2.3.1)
  - Downloading drupal/real_aes (2.4.0)
  - Installing drupal/key (1.15.0): Extracting archive
  - Installing drupal/encrypt (3.0.0): Extracting archive
  - Installing paragonie/random_compat (v9.99.100): Extracting archive
  - Installing defuse/php-encryption (v2.3.1): Extracting archive
  - Installing drupal/real_aes (2.4.0): Extracting archive
1 package suggestions were added by new dependencies, use `composer suggest` to see details.
Package doctrine/reflection is abandoned, you should avoid using it. Use roave/better-reflection instead.
Generating autoload files
41 packages you are using are looking for funding.
Use the `composer fund` command to find out more!

That seemed to find the dependency on "drupal/encrypt": "^3.0" even for an older version that did not have this patch in it. Three contribs were installed:

ls -l web/modules/contrib/
total 0
drwxr-xr-x  19 greggles  staff  608 Oct 10  2020 encrypt
drwxr-xr-x  20 greggles  staff  640 Oct 12 15:57 key
drwxr-xr-x   8 greggles  staff  256 Jan 20 07:22 real_aes

When I removed real_aes it seemed to detect some dependencies and remove them:

composer remove 'drupal/real_aes'
./composer.json has been updated
Running composer update drupal/real_aes
Loading composer repositories with package information
Updating dependencies
Lock file operations: 0 installs, 0 updates, 5 removals
  - Removing defuse/php-encryption (v2.3.1)
  - Removing drupal/encrypt (3.0.0)
  - Removing drupal/key (1.15.0)
  - Removing drupal/real_aes (2.4.0)
  - Removing paragonie/random_compat (v9.99.100)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 0 installs, 0 updates, 5 removals
  - Removing paragonie/random_compat (v9.99.100)
  - Removing drupal/real_aes (2.4.0)
  - Removing drupal/key (1.15.0)
  - Removing drupal/encrypt (3.0.0)
  - Removing defuse/php-encryption (v2.3.1)
 0/5 [>---------------------------]   0%Deleting web/modules/contrib/real_aes - deleted
 3/5 [================>-----------]  60%Deleting web/modules/contrib/encrypt - deleted
Deleting web/modules/contrib/key - deleted
Package doctrine/reflection is abandoned, you should avoid using it. Use roave/better-reflection instead.
Generating autoload files
41 packages you are using are looking for funding.
Use the `composer fund` command to find out more!

And after running that remove the contribs were all removed as were 3 items from vendor (generate-defuse-key, defuse, paragonie).

So, it seems from this research that installing a version of the module from drupal.org's composer facade is working as I expect it to, but I imagine there may be some scenarios where that doesn't work as I experienced.

Hoping to learn more as we discuss this. Thanks.

rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented

@greggles - If you executed composer require 'drupal/real_aes:^2.2', the latest version (2.4.0) was probably installed. That's what happened when I tried it. I forced the version before the patch to be installed with composer require 'drupal/real_aes:2.3' and observed the same dependencies being downloaded as you did.

jcnventura is right that Drupal's Composer packagist does seem to do some magic to make dependencies defined in the info.yml file proper dependencies (as far as Composer is concerned) during installation, which in my test resulted in installation of Encrypt, which caused Key to be installed, even though Key is only in Encrypt's .info.yml file, not its Composer file. I did not have to require drupal/encrypt and drupal/key to get that to happen.

However, I'd prefer to be explicit about dependencies in the convention of modern PHP projects, which means specifically defining them in composer.json.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Great catch, @rlhawk! That is indeed what happened. And when I did it with composer require 'drupal/real_aes:2.2' I got an error since that version is not compatible with the Drupal 9 that my test project was running. But indeed 2.3 worked fine as you noted.

I agree with your perspective that it's fine to be more explicit. It does mean there's some risk of the 2 files drifting away from each other and specifying some slightly different versions or dependencies, but that feels pretty low risk and easy to fix.

jcnventura’s picture

jcnventura
Primary language English
commented

@greggles, in my experience the two files drifting away from each other is exactly the real problem here. That's why it's better to define things in a single place.

I too would rather that the magic would disappear from Drupal packagist, and that module maintainers would be able to only specify dependencies via composer.json, and that the .info.yml file was optional, but at this moment it is the composer.json file that is optional.

Status: Fixed » Closed (fixed)

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