Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drush.services.yml is picked even when requirements are not met #5600

Closed
rodrigoaguilera opened this issue May 30, 2023 · 17 comments · Fixed by #5605
Closed

drush.services.yml is picked even when requirements are not met #5600

rodrigoaguilera opened this issue May 30, 2023 · 17 comments · Fixed by #5605

Comments

@rodrigoaguilera
Copy link
Contributor

rodrigoaguilera commented May 30, 2023

Describe the bug
Specifying a drush.services.yml file in the composer.json of a Drupal module with a restriction that is not fulfilled (such as ^999) loads the classes regardless.

To Reproduce
Change the restriction in a composer.json file of a module to ^999 and verify the commands defined in the drush.services.yml are still loaded.

Expected behavior
The drush.services.yml is ignored

Actual behavior
The commands in the drush.services.yml file are loaded

Workaround
Specify an non-existing yml.

    "extra": {
        "drush": {
            "services": {
                "non-existent.services.yml": ">11.99",
                "drush11.services.yml": "<11.99"
            }
        }
    }

System Configuration

Q A
Drush version? 12.0.0-rc3
Drupal version? 10.1.0-beta1
PHP version 8.1
OS? Linux

Additional information
I'm trying to provide a backwards compatible way to add v3 generators to the extra_field Drupal module
https://git.drupalcode.org/project/extra_field/-/blob/8.x-2.x/drush.services.yml
and I'm looking for a way to ignore the v1 and v2 generators. I think I'm going to use the workaround so far but I found the behavior odd.

It is probably not an easy fix since many Drupal modules declare the services file with something like "drush.services.yml": "^10" but that won't match drush 11 but currently it works for drush 11 because one of the files is always picked.

@weitzman
Copy link
Member

weitzman commented May 30, 2023

Thanks for providing generator versions for everyone!

As of last week, Drush 12 generators ignore any drush.services.yml. Use the create() method to get dependencies from the drupal container. This is great but it does make it harder to do this multiple versions thing. Maybe @greg-1-anderson has an idea besides your clever workaround.

Docs are at https://www.drush.org/12.x/generators/

@greg-1-anderson
Copy link
Member

I think my preference for version-specific commands and generators would be to use a different major version for your module / library for each version of Drupal that you support, and use a conflicts entry to prevent using the wrong version of your package with the wrong version of Drupal.

@weitzman
Copy link
Member

weitzman commented May 30, 2023

I don't think that approach works well anymore. Drupal modules routinely span Drupal versions. The OP wants to span multiple versions of Drush (actually DCG), not Drupal. Unlike commands, the Generator API is now on v3, in just a few years.

@rodrigoaguilera
Copy link
Contributor Author

Do you think I should open an issue in DCG to figure out how to support v2 and v3 at the same time in the same branch of a module?

@weitzman
Copy link
Member

I think we can use this very issue, since Drush's implementation of DCG is a big factor. Ping @Chi-teck

@Chi-teck
Copy link
Collaborator

As of last week, Drush 12 generators ignore any drush.services.yml.

Does this work this way in Drush 12.0.0-RC3 ?

@weitzman
Copy link
Member

Yes.

@greg-1-anderson
Copy link
Member

The same technique should work fine, if you use different major version for your module / library for each version of DCG that you support, and then use conflicts or requires to pin to the correct major version of DCG that you need.

@rodrigoaguilera
Copy link
Contributor Author

That means that if a module like extra_field wants to start supporting v3 DCG they need to release a new major version where the support for v2 is dropped?

@greg-1-anderson
Copy link
Member

greg-1-anderson commented May 31, 2023

OK you're right, that's not great, because you'd otherwise want the rest of the module to be the same between the major versions.

  1. You could put your Drush command in a separate library, e.g. in packagist, and use the major version of the Drush command library to select the DCG version you need. That would work, but maybe it would be inconvenient to have a separate library in a separate package. (Pro: Uses standard features only. Con: Inconvenient for module maintainers)

  2. You could also use a path repository in your module, and expose multiple versions of your Drush command in different directories, again using conflict to ensure that Composer only selects one of these projects, based on the DCG version. That's probably possible, but I haven't set it up, and therefore this technique should be considered unproven. Even if it worked fine (which I expect it does), it's still a little convoluted. (Pro: Uses standard features only. Con: These features are not commonly used or understood, and would be difficult to set up; once set up, though, would be more convenient than option1)

  3. We could consider adding a feature to Drush, where we add a custom autoloader for each module that contains a well-known directory name that includes the Drush or DCG major version that it worked with. (Pro: Convenient for maintainers. Con: Extra complexity for Drush, and extra nesting for module command file paths)

  4. We could also consider adding a feature to command discovery (I forget if that's in Robo or the Annotated Command library) that would allow Drush to pass a major version number to the classname search algorithm, allowing it to favor class names that end with the right major version, similar to the Drush version-specific filename matching from Drush 8 and earlier. (Pro: Convenient for maintainers. Con: There are Drush major version numbers in your class names.)

Which of these options sounds best to folks?

@greg-1-anderson
Copy link
Member

The above options are not relevant if we come up with a good way for the same command file to support multiple major versions of DCG.

@Chi-teck
Copy link
Collaborator

Chi-teck commented May 31, 2023

The issue summary raises a valid question. Why does Drush load services file when its requirements are not net? If we could ignore drush11.services.yml, then it wouldn't be a problem to support multiple versions of DCG simultaneously.

The other possible approach is not loading services tagged with drush.generator.v2.

@greg-1-anderson
Copy link
Member

Drush stopped ignoring the version constraints on the default services file, drush.services.yml, because it was common for standard Drush commands to be compatible with new major versions of Drush, but they still could not be used until the version constraint was bumped, resulting in a lot of toil. The version constraint is / would be more useful for generators, perhaps. The other issue we have, though, is that we are deprecating drush.services.yml in favor of the static create method in Drush 12. As mentioned above, generators cannot even be loaded from drush.services.yml in Drush 12, although commands are still loaded for backwards compatibility. Drush 11 used DCG ^2, and Drush 12 uses ^3, so there is no backwards compatibility motivation to support drush.services.yml for generators in Drush 12. I don't know if we want to consider re-introducing drush.services.yml for generators in Drush 12 due to the utility of the feature.

@weitzman
Copy link
Member

weitzman commented May 31, 2023

I don't know if we want to consider re-introducing drush.services.yml for generators in Drush 12 due to the utility of the feature.

I dont especially want to re-introduce that.

For anyone who isn't satisfied with the workaround in this issue, fix Drush 11 constraint handling so that ^999 constraint works as expected.

@Chi-teck
Copy link
Collaborator

The other possible approach is not loading services tagged with drush.generator.v2.

How about this way? See #5605.

@greg-1-anderson
Copy link
Member

So, if we just merge #5605, then we never backport static factory create support for generators to Drush 11, and folks can then use static create in Drush 12 / DCG 3, and drush.services.yml with the tag drush.generator.v2 in Drush 11 / DCG 2? And we worry about how to handle DCG 4 when it happens?

@rodrigoaguilera
Copy link
Contributor Author

With that PR I don't think the issue in the title needs a fix since drush.services.yml will be deprecated anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants