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

Missing services are quietly ignored #5195

Closed
chx opened this issue Jul 31, 2022 · 10 comments · Fixed by #5204
Closed

Missing services are quietly ignored #5195

chx opened this issue Jul 31, 2022 · 10 comments · Fixed by #5204

Comments

@chx
Copy link
Contributor

chx commented Jul 31, 2022

Describe the bug

I am debugging a new project and they didn't send a settings.php and the one I put together accidentally ommitted $settings["config_sync_directory"]. drush simply omits the config import and export commands and there is no error message anywhere. I only realized when I gave up and went to /admin/config/development/configuration in the UI.

To Reproduce
What did you do?

Oh dear, what have I done, yeah, that's what I am asking myself in the long lonely nights. But in this case see above.

Expected behavior

Maybe something in drush status?

Actual behavior
What happened instead?

drush config:import is just missing

Workaround
Is there another way to do the desired action?

Not really.

@chx chx changed the title No error message for $settings["config_sync_directory"] messing No error message for $settings["config_sync_directory"] missing Jul 31, 2022
@weitzman
Copy link
Member

weitzman commented Aug 1, 2022

The commands in src/Drupal require a bootstrapped Drupal in order to function. You dont have that without a config sync dir. We do provide the following error in that case

Command !command was not found. Drush successfully connected to the database but was unable to fully bootstrap your site. As a result, many commands are unavailable. Re-run your command with --debug to see relevant log messages.

Please post here if you aren't seeing that error. I will reopen the issue.

@weitzman weitzman closed this as completed Aug 1, 2022
@weitzman
Copy link
Member

weitzman commented Aug 1, 2022

Hmm, or maybe Drupal does bootstrap fine without a sync dir?

@chx
Copy link
Contributor Author

chx commented Aug 1, 2022

Oh yes it does. drush status show full bootstrap, drush php runs fine, almost all Drupal pages load fine. Except the config sync page.

@weitzman
Copy link
Member

weitzman commented Aug 1, 2022

I think then it might be Drupal's service container which wont allow the registration of this command without its dependant argument. Not sure.

@weitzman weitzman reopened this Aug 1, 2022
@chx
Copy link
Contributor Author

chx commented Aug 1, 2022

So ... trying to initiate the config.storage.sync service will throw an exception and indeed the import command depends on this.

The question becomes then, what catches this exception? It showing up would be the very error message message missing here. And, this might mean there's a much wider bug here of something catching an exception that would be better shown? At least in verbose, if it is desired to silently just not show commands which can't be made.

@andypost
Copy link
Contributor

andypost commented Aug 1, 2022

It smells as core issue

@chx
Copy link
Contributor Author

chx commented Aug 1, 2022

Smells like drush to me.

In FindCommandsCompilerPass there's a [new Reference($id, ContainerInterface::IGNORE_ON_INVALID_REFERENCE)].

Somehow we need to warn people when this causes a service to be ignored. Could be something that happens only verbose, as I mentioned above.

@chx chx changed the title No error message for $settings["config_sync_directory"] missing Missing services are quietly ignored Aug 1, 2022
@weitzman
Copy link
Member

This was added recently. See #5056. @DieterHolvoet - perhaps we should revert that PR and find some other solution?

@DieterHolvoet
Copy link
Contributor

The reason I added ContainerInterface::IGNORE_ON_INVALID_REFERENCE was to make it possible to remove registered commands in service providers. Like this:

<?php

namespace Drupal\some_module;

use Drupal\Core\DependencyInjection\ContainerBuilder;
use Drupal\Core\DependencyInjection\ServiceModifierInterface;

class SomeModuleServiceProvider implements ServiceModifierInterface
{
    public function alter(ContainerBuilder $container)
    {
        if (!$container->has('some_service')) {
            $container->removeDefinition('some_module.some_command');
        }
    }
}

Without ContainerInterface::IGNORE_ON_INVALID_REFERENCE, having the above code would result in this error:

In CheckExceptionOnInvalidReferenceBehaviorPass.php line 86:

  The service "drush.command.services" has a dependency on a non-existent service "some_module.some_command".

Any command that was once registered through a drush.commands.yml file could never be removed from the container afterwards, because drush.command.services has a hard dependency on all registered commands.

We could revert that change, or we could fix this specific issue where a missing config_sync_directory isn't reported. I'll create a PR for the latter.

@weitzman
Copy link
Member

Lets discuss in the PR

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