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 Monolog v2 to enable psr/log v2/v3 #1822

Merged
merged 2 commits into from
Jan 12, 2022
Merged

Conversation

dereuromark
Copy link
Contributor

@dereuromark dereuromark commented Jan 10, 2022

When looking into dependencies, it becomes clear that psr/log v1 is required due to a require-dev (optional) dependendy locking it
We need to prevent this in order to fully test both our min and max promise.

As such, this PR aims to fix it properly - and replaces #1819

Completes #1805

With this PHP7 folks are using psr/log v1, PHP8 folks can use psr/log v2/v3 already.

@oojacoboo
Copy link
Contributor

oojacoboo commented Jan 10, 2022

Is it necessary for this lib to define the psr/log interface dependency in composer.json if the only dependency to this interface exists in a 3rd party lib/dep?

@dereuromark
Copy link
Contributor Author

dereuromark commented Jan 10, 2022

Probably not, in that case we could move this also into require-dev and unblock people then fully if no such library is used.
I wonder if "any logger"/"at least one" would still be a good requirement by default though.
Is anyone using this lib without a logger in place at all?

@oojacoboo
Copy link
Contributor

If the code doesn't require the interface, we shouldn't be having it as a dependency. The dependencies should be for the direct use in this lib. I assumed Propel needed that interface for it's own internal logging logic.

If that's not the case, it should be removed entirely, allowing for monolog/monolog to manage it's own dependencies, however necessary.

@dereuromark
Copy link
Contributor Author

dereuromark commented Jan 10, 2022

class ConnectionWrapper implements ConnectionInterface, LoggerAwareInterface

so this is quite deeply integrated.

Given that I don't think we can move it into require-dev.

With this PR all constraints are still unblocked as everyone can use the correct psr interfaces as per their needs.
So we should be fine.

@oojacoboo
Copy link
Contributor

The issue is this:

    /**
     * @param \Psr\Log\LoggerInterface $logger
     *
     * @return void
     */
    public function setLogger(LoggerInterface $logger)
    {
        $this->logger = $logger;
    }

https://github.com/php-fig/log/blob/2.0.0/src/LoggerInterface.php

Version 2 there of the LoggerInterface uses union types. That's a PHP 8 feature. Therefore, version 2 and version 3 of this interface cannot be supported without also supporting a min of PHP 8.

@oojacoboo
Copy link
Contributor

This PR is fine though, I agree. #1819 still needs merging as well.

@dereuromark dereuromark merged commit 39203e1 into master Jan 12, 2022
@dereuromark dereuromark deleted the bugfix/psr-log-v2-v3 branch January 12, 2022 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants