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

Allow to set the URL for the security advisories composer url via env variable #5180

Merged

Conversation

Cyberschorsch
Copy link
Contributor

@Cyberschorsch Cyberschorsch commented Jul 11, 2022

The URL which is used in the drush pm:security command is hard-coded and links to the drupal-security-advisories repo ( https://github.com/drupal-composer/drupal-security-advisories ). I really would like to have an easy option to use a different url if needed.

This PR sets the url for the source of the security check command to the environment variable DRUSH_SECURITY_ADVISORIES_URL if set, otherwise use the default url.

@weitzman
Copy link
Member

Seems reasonable.

  • Please add a comment about this env variable not being a supported API.
  • Pkease use the null coalesce php operator to save some characters,

@Cyberschorsch
Copy link
Contributor Author

@weitzman thank your for the fast feedback, really appreciate it. I made the changes you suggested.

@weitzman
Copy link
Member

I see that the tests are failing. Hopefully its a quick fix.

@Cyberschorsch
Copy link
Contributor Author

Since getenv() returns FALSE instead of NULL if the env variable is not set or empty we propably have to stick with the ternary operator.

Copy link
Member

@weitzman weitzman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i was thinking of ternary. here is a suggested change

src/Commands/pm/SecurityUpdateCommands.php Outdated Show resolved Hide resolved
Co-authored-by: Moshe Weitzman <weitzman@tejasa.com>
@weitzman weitzman merged commit f324ff5 into drush-ops:11.x Jul 12, 2022
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 this pull request may close these issues.

None yet

2 participants