Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Set boolean to class_implements #204

Merged
merged 2 commits into from
May 4, 2021
Merged

Conversation

doox911-opensource
Copy link
Contributor

Why class name?

https://www.php.net/manual/en/function.class-implements.php

The second parameter is boolean. I understand that there will be a cast, but still.

https://www.php.net/manual/en/function.class-implements.php

The second parameter is boolean. I understand that there will be a cast, but still.
@@ -15,7 +15,7 @@ public function __construct(
public string $casterClass,
mixed ...$args
) {
if (! class_implements($this->casterClass, Caster::class)) {
if (! class_implements($this->casterClass, true)) {
Copy link
Contributor

@jdreesen jdreesen Apr 30, 2021

Choose a reason for hiding this comment

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

Should be this then, I think:

Suggested change
if (! class_implements($this->casterClass, true)) {
if (! in_array(Caster::class, class_implements($this->casterClass, true), true) {

Or maybe:

Suggested change
if (! class_implements($this->casterClass, true)) {
if (! is_a($this->casterClass, Caster::class, true)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant that it will always be true here. A list of interfaces is also required here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right. But I don't think the check makes sense at all. Would be strange to check if $this->casterClass just implements something. The intention was probably to check if it implements Caster::class.

@brendt
Copy link
Contributor

brendt commented May 3, 2021

That's absolutely my bad; it should have been is_subclass_of. I'll fix this later this week.

@brendt  I corrected, in this case it will be possible to accept my PR?
@@ -15,7 +15,7 @@ public function __construct(
public string $casterClass,
mixed ...$args
) {
if (! class_implements($this->casterClass, Caster::class)) {
if (! is_subclass_of($this->casterClass, Caster::class)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need the third parameter to be true here, because the first one is a string.

Suggested change
if (! is_subclass_of($this->casterClass, Caster::class)) {
if (! is_subclass_of($this->casterClass, Caster::class, true)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What for? There is true by default.

Copy link
Contributor

@jdreesen jdreesen May 3, 2021

Choose a reason for hiding this comment

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

Oh, you're right! I was confused because for is_a() it's default false....

@brendt brendt merged commit 1dfd302 into spatie:master May 4, 2021
@brendt
Copy link
Contributor

brendt commented May 4, 2021

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants