thephpleague / tactician-bundle

Bundle to integrate Tactician with Symfony projects
MIT License
245 stars 43 forks source link

Symfony 3.4: By default all services are private. #95

Closed ZhukV closed 6 years ago

ZhukV commented 6 years ago

Hi, in own project, after the upgrade the Symfony to 3.4 version, we have many deprecated notices after running functional tests.

Origin problem: https://symfony.com/blog/new-in-symfony-3-4-services-are-private-by-default

The "tactician.commandbus.default" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop using the container directly and use dependency injection instead:
rosstuck commented 6 years ago

Hi @ZhukV, sorry to hear about that! Can you let me know what version of the Tactician bundle you're using? As far as I know, we're setting all the command bus instances to public already (tactician.commandbus.default is a bus instance, not a handler).

ZhukV commented 6 years ago
league/tactician: v1.0.3
league/tactician-bundle: v0.4.1
ZhukV commented 6 years ago

I see, what the command bus fixed for Symfony 4 (set the public flag). But, how we should fix the command handlers? Set the public: true to each handler?

ZhukV commented 6 years ago

Oh, sorry, I do not see new dependent league/tactician-container.

rosstuck commented 6 years ago

Hmm, you're using a very old version of the bundle, can you upgrade to ~1.1 and see if the issue persists? I have a feeling this was resolved earlier but don't recall off the top of my head.

adiq commented 6 years ago

I can confirm that issue still persist with:

league/tactician                           v1.0.3  A small, flexible command bus. Handy for building service layers.
league/tactician-bundle                    v1.1.2  Bundle to integrate Tactician with Symfony projects

Services should be renamed to use FQCN and be private. I guess aliases could be added as public to have backward compatibility (see https://symfony.com/doc/current/service_container/3.3-di-changes.html#step-2-using-class-service-id-s).

rosstuck commented 6 years ago

Thanks for reporting a similar error, @adiq. I've looked into this but we have no reported failures on 3.4 which has been on our test suite for quite a while.

I've created a new 3.4 app from scratch to investigate and I can't reproduce this error anywhere, unfortunately.

Relevant parts of my container config looks like:

_defaults:
        autowire: true    
        autoconfigure: true 
        public: false    

App\Controller\TestController:
        arguments:
            - '@tactician.commandbus.default'

    App\Domain\:
        resource: "../src/Domain"
        public: false
        tags:
            - { name: tactician.handler, typehints: true }

My tactician config is the standard default one, comments removed:

tactician:
    commandbus:
        default:
            middleware:
                - tactician.middleware.command_handler

Using a non-autowired version also worked:

App\Domain\ReportBugHandler:
        tags:
            - { name: tactician.handler, command: App\Domain\ReportBug }

I've tried to do the most standard, out of the box install possible.

@ZhukV Can you confirm if upgrading your dependencies worked? @adiq Can you confirm you're getting the exact same error message as @ZhukV? Can both of you take a look at your configs and see if you can produce a unit or integration test here? I'm happy to follow up on this but I can't reproduce at this time.

Thanks!

ZhukV commented 6 years ago

@rosstuck after upgrade to 1.1.2 all application correct works on Symfony 3.4 application (without autowiring).

adiq commented 6 years ago

I have been a little misleading, it does not cause any fatal error but throws deprecation message as it would be incompatible with Symfony 4. I will try to verify all that in upcoming days and will get back to you with more details ;-)

rosstuck commented 6 years ago

To my knowledge, our Symfony 4 support is okay and I'm not seeing any deprecation notices in the quick and dirty 3.4 install I made. Not saying it isn't there, just that there may be an edge case factor.

If you can provide any more information, please let me know. Otherwise, I'll close this issue soon since it doesn't seeeeeem to be the same issue that was originally reported. Thanks again!

rosstuck commented 6 years ago

Closing due to inactivity. Thanks everyone for your contributions!