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

Make config.storage.sync injection optional and log any issues #5204

Conversation

DieterHolvoet
Copy link
Contributor

Fixes #5195.

@weitzman
Copy link
Member

weitzman commented Sep 3, 2022

@bircher @greg any thoughts? See linked issue. @greg-1-anderson

@bircher
Copy link
Contributor

bircher commented Sep 7, 2022

I am ok with this approach. But I think we need to throw the exception also when trying to use the uninitialized service. Because otherwise it will be difficult to understand why the command doesn't work.

@greg-1-anderson
Copy link
Member

I don't have time to try it myself right now. The things that are unclear to me from the linked issue:

  1. What happens in the Drupal admin UI when the config sync directory is undefined? Do any import / export functions work?
  2. What happens in Drush if you run config import or export and specify a directory rather than relying on the config sync configuration, when the later is not defined?

Having an indicator in drush status seems like it would be very helpful to have as well.

@DieterHolvoet DieterHolvoet force-pushed the 5195-missing-services-are-quietly-ignored branch from 9d50715 to 4ff5d46 Compare September 10, 2022 06:46
@DieterHolvoet
Copy link
Contributor Author

I am ok with this approach. But I think we need to throw the exception also when trying to use the uninitialized service. Because otherwise it will be difficult to understand why the command doesn't work.

@bircher do you mean adding a check in getConfigStorageSync()?

@bircher
Copy link
Contributor

bircher commented Sep 21, 2022

Ok so I checked more closely again. And we should refactor this a lot. I don't know what part of this is considered "public api of drush" But I don't think it is a good idea to extend the import and export command classes.
Our code should work fine even when not checking for the property to be set because the code works with the path to the sync directory and checks if the path is the same as the one in the settings before accessing the sync storage property. So my concern from the previous comment is mitigated. And since I don't think these methods should be API it is fine to do without the check since php will already complain by itself.

@bircher
Copy link
Contributor

bircher commented Sep 21, 2022

so tldr: looks good to me

@weitzman weitzman enabled auto-merge (squash) September 21, 2022 14:15
@weitzman weitzman merged commit 5ec59e2 into drush-ops:11.x Sep 21, 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.

Missing services are quietly ignored
4 participants