Have LoggerServiceFactory inject writer and processor plugin managers

[TotalWipeOut]

Q	A
Documentation	no
Bugfix	yes
BC Break	no
New Feature	no

Description

The issue: LoggerServiceFactory does not inject the writer nor the processor plugin managers. But LoggerAbstractServiceFactory does. And I want to use the former with custom processors.

I changed the inheritance so LoggerAbstractServiceFactory extends LoggerServiceFactory and moved some logic, so both inject the relevant plugin managers in the same way.

I changed PsrLoggerAbstractAdapterFactory to compose LoggerAbstractServiceFactory, not inherit. It didn't make sense as the return types for __invoke were incompatible. So adding strict typing across much of the factories would be possible now

Added unit tests for LoggerServiceFactory. All other unit tests still pass, and the factories can still be used in exactly the same way. Only difference is now LoggerServiceFactory injects the plugin managers.

Typing only added to anything new to maintain BC

I want to target the 2.15.x branch, as i see this as a bugfix - however, i am new to open source, and am open to guidance.

[Ocramius]

@TotalWipeOut probably need only a086d99, right?

[Ocramius]

Tests are needed for this addition.

[TotalWipeOut]

Thanks @Ocramius - yes only meant that one commit, i will redo this.

OK, i will write some tests for this change - thanks

[Ocramius]

@TotalWipeOut you can force-push a cleaned up branch to your own PR branch BTW - no need to re-create this.

[TotalWipeOut]

I have added tests for LoggerServiceFactory, the other factories were already fully tested. I have removed all v2 API methods from factories too. I am not sure why there are failing checks - they seem unrelated to my changes, but do say if there is an error somewhere. thanks

[froschdesign]

@TotalWipeOut

I have removed all v2 API methods from factories too.

This also means to remove the interface Interop\Container\ContainerInterface.

[TotalWipeOut]

@froschdesign

@TotalWipeOut

I have removed all v2 API methods from factories too.

This also means to remove the interface Interop\Container\ContainerInterface.

I dont consider that v2 API, and because composer.json is set to "laminas/laminas-servicemanager": "^3.3.0", and it was version 3.11.0 where the alias was added for Interop to Psr, to not BC break the interfaces

Changing Interop to Psr would require bumping version in composer.json, and i don't really want to change deps

Assuming i understood this correctly :)

[froschdesign]

@TotalWipeOut laminas-servicemanager 3.3.0 uses container-interop/container-interop with version 1.2.0 and this means:

namespace Interop\Container;

use Psr\Container\ContainerInterface as PsrContainerInterface;

interface ContainerInterface extends PsrContainerInterface
{
}

https://github.com/container-interop/container-interop/blob/79cbf1341c22ec75643d841642dd5d6acd83bdb8/src/Interop/Container/ContainerInterface.php#L13

Changing Interop to Psr would require bumping version in composer.json, and i don't really want to change deps

Why not? After all, you are already working on this package, even though it is in security-only maintenance mode.

[TotalWipeOut]

@froschdesign https://github.com/laminas/laminas-servicemanager/blob/3.3.0/src/Factory/FactoryInterface.php#L11 The FactoryInterface requires Interop, the inheritance doesn't fix that. Using Psr namespace breaks it, try yourself using servicemanager 3.3.0 :)

The only way laminas was able to move to Psr, was via aliasing: https://github.com/laminas/laminas-servicemanager/blob/3.11.x/src/autoload.php

[TotalWipeOut]

Why not? After all, you are already working on this package

I just wanted to fix this bug, not refactor the package. Also, the current project i am working on is currently stuck on servicemanager 3.10.0 for other reasons, so doing this would push my fix out of my reach lol

[froschdesign]

@TotalWipeOut My comment was misleading: you should not use version 3.3.0 of laminas-servicemanager, instead raise the version.

[TotalWipeOut]

Ah, you know, I hadn't actually noticed that security only message :man_facepalming: So on those grounds, would this bugfix not be accepted...?

[froschdesign]

So on those grounds, would this bugfix not be accepted...?

All good, your changes will be accepted with thanks.

[TotalWipeOut]

@weierophinney thank you for the review. Understood. I will rollback the v2 changes and return types

[TotalWipeOut]

@weierophinney I have, hopefully, addressed all your comments. I hope you can spare some time to check them out. Thanks in advance.

also, unsure why some of the github checks are failing, the reason shown i cannot replicate and doesn't seem related to my changes...

[TotalWipeOut]

@weierophinney @Ocramius Would you have any time this week to finish off this PR? Thanks

[Ocramius]

Hmm, what about the CI failures here? :thinking:

As for the rest, I can merge + release once @weierophinney gives a :+1: on this

[TotalWipeOut]

@Ocramius I only just realised earlier, that latest meant composer update the package, and not using latest php patch version :see_no_evil: sorry

From this, I could see that laminas-validator 2.25.0 had broken the laminas-logs, Filter/Validator class, because ValidatorChain was now an iterator - which I have fixed! :tada:

CI now passing, except the Autocloser, which looks broken?