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 a constructor to CacheKey #10212

Merged

Conversation

derrabus
Copy link
Member

Prepares #10210 on 2.14.x.

@derrabus derrabus requested a review from greg0ire November 10, 2022 09:30
@derrabus derrabus changed the title Add a constructor to CacheKey Add a constructor to CacheKey Nov 10, 2022
@derrabus derrabus added this to the 2.14.0 milestone Nov 10, 2022
@derrabus derrabus force-pushed the improvement/cache-key-constructor branch from da62e25 to f7f13a4 Compare November 10, 2022 09:41
public function __construct(string $hash)
{
$this->hash = $hash;
parent::__construct($hash);
}
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Can't you remove the constructor entirely instead? Or maybe even the whole class is useless?

Copy link
Member

Choose a reason for hiding this comment

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

Actually we would need a test with constructor and without, because despite the deprecation both ways need to work.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 Can't you remove the constructor entirely instead?

I can, but I was reluctant to do so because it's stricter than CacheKey's.

Or maybe even the whole class is useless?

CacheKey is abstract and I don't really want to change that. So if we dropeed CacheKeyMock, I would need to replace it with an anonymous class in every test that uses it. I'm not sure that's an improvement really.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually we would need a test with constructor and without, because despite the deprecation both ways need to work.

I've pushed two new tests.

Copy link
Member

Choose a reason for hiding this comment

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

Didn't notice the extra strictness 👍

@derrabus derrabus force-pushed the improvement/cache-key-constructor branch from f7f13a4 to a018ad5 Compare November 11, 2022 09:16
@derrabus derrabus merged commit 953e42d into doctrine:2.14.x Nov 11, 2022
@derrabus derrabus deleted the improvement/cache-key-constructor branch November 11, 2022 09:50
derrabus added a commit to derrabus/orm that referenced this pull request Nov 11, 2022
* 2.14.x:
  Add a constructor to CacheKey (doctrine#10212)
  Psalm 4.30.0, PHPStan 1.9.2 (doctrine#10213)
  Allow "Expr\Func" as condition in join (doctrine#10202)
  refactor: use list type in SchemaTool (doctrine#10199)
derrabus added a commit to derrabus/orm that referenced this pull request Nov 11, 2022
* 2.14.x:
  Add a constructor to CacheKey (doctrine#10212)
  Psalm 4.30.0, PHPStan 1.9.2 (doctrine#10213)
  Allow "Expr\Func" as condition in join (doctrine#10202)
  refactor: use list type in SchemaTool (doctrine#10199)
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