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

Add parameter types #455

Merged
merged 2 commits into from
Dec 14, 2023
Merged

Add parameter types #455

merged 2 commits into from
Dec 14, 2023

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Dec 14, 2023

In DoctrineMongoDBBundle, we can't add parameter types to all the classes because they are not defined in the base class or the interface. https://github.com/doctrine/DoctrineMongoDBBundle/pull/812/files#diff-1571e1997e046b65a3fdffae3826e46e2a5a89adcce01ff580557d7eedc536e1R71

Adding parameter types to all the methods should not be an issues for classes that extends them. But it might for interfaces.

Also upgrading to PHP 8.1 is required for some mixed and union types.

@derrabus
Copy link
Member

This PR does too many things. Let's start small please.

Let's start with parameter types only. Bumping to 8.1 does not yet feel necessary to me. We should not have many union types in parameters and mixed is the only parameter type that you may add downstream.

@derrabus derrabus changed the base branch from 1.7.x to 1.8.x December 14, 2023 15:01
@GromNaN
Copy link
Member Author

GromNaN commented Dec 14, 2023

Indeed, I went too far. Reworking.

@GromNaN
Copy link
Member Author

GromNaN commented Dec 14, 2023

It did not change the type for ORMPurger::getJoinTableName which requires an union ``.
Also ReferenceRepository::setReferenceIdentity has a `mixed` argument.

Nothing needs to be updated in tests.

@GromNaN GromNaN changed the title Add parameter types and update CS for PHP 8.1 Add parameter types Dec 14, 2023
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far. Once this PR is merged, we might open a 2.0 branch where we bump to 8.1 and apply return types, WDYT?

src/AbstractFixture.php Outdated Show resolved Hide resolved
src/AbstractFixture.php Outdated Show resolved Hide resolved
* @return void
*/
public function setLogger($logger)
public function setLogger(callable $logger)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the logger a callable anyway? 🙈

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 94.

        $logger = $this->logger;
        $logger($message);

You think we should change it to accept a Psr\Log\LoggerInterface?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should look into that, yes. But probably in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's used for console output directly.

We could try to leverage Symfony\Component\Console\Logger\ConsoleLogger. My problem is that a closure is a bad logging abstraction. While it works for the way the bundle uses it, it sucks if you want the logs to go anywhere else, e.g. you want to use a logging framework instead.

Deprecation: #462

Co-authored-by: Alexander M. Turek <me@derrabus.de>
@GromNaN
Copy link
Member Author

GromNaN commented Dec 14, 2023

Once this PR is merged, we might open a 2.0 branch where we bump to 8.1 and apply return types, WDYT?

Sounds like a good idea.
There is little deprecated things to remove, and we could add return types.

@derrabus derrabus added this to the 1.8.0 milestone Dec 14, 2023
@greg0ire greg0ire merged commit dee43b2 into doctrine:1.8.x Dec 14, 2023
13 checks passed
@greg0ire
Copy link
Member

Thanks @GromNaN !

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