Getting properties based on whether they have a given attribute

[NotionCommotion]

For all objects that implement HasRankedListInterface, I would like to retrieve all of their properties that have the RankedListAttribute.

class Project implements HasRankedListInterface
{
    #[RankedListAttribute]
    private ?ProjectStage $projectStage = null;

    public function getProjectStage(): ?ProjectStage
    {
        return $this->projectStage;
    }
}

class ProjectStage extends AbstractRankedList
{
}

abstract class AbstractRankedList implements RankedListInterface
{
}

use Attribute;

#[Attribute(Attribute::TARGET_METHOD | Attribute::TARGET_FUNCTION | Attribute::IS_REPEATABLE)]
class RankedListAttribute
{
}

My approach below seems a little like a hack.  Should I be doing differently?  Thanks

    public function updateUserListRanking(ViewEvent $viewEvent): void
    {
        $object = $viewEvent->getControllerResult();

        if (!is_object($object)) {
            return;
        }

        if (!$object instanceof HasRankedListInterface) {
            return;
        }

        $classReflect = new \ReflectionClass($object);
        $properties = [];
        foreach ($classReflect->getProperties() as $propReflect) {
            foreach ($propReflect->getAttributes() as $attrReflect) {
                if($attrReflect->getName() === RankedListAttribute::class) {
                    $property =  $object->{'get'.ucfirst($propReflect->getName())}();
                    if (!$property instanceof RankedListInterface) {
                        throw new \Exception($propReflect->getType().' must implement RankedListInterface');
                    }
                    $properties[] = $property;
                }
            }
        }

        if (!$properties) {
            return;
        }

        $user = $this->getUser();
        $this->doSomething($user, ...$properties);
    }

 

[requinix]

Reflection, list of properties, list of attributes given that there is no "hasAttribute"... yeah, that's about right.

You could streamline the attribute search with a quick array_filter instead of a foreach:

$hasAttr = array_filter($propReflect->getAttributes(), fn($attr) => $attr->getName() === RankedListAttribute::class);

 

[NotionCommotion]

Thanks.  What about my ugly getter which is based on naming standards?  Also thought that the logic for enforcing RankedListInterface for all properties with RankedListAttribute could/should be done elsewhere.

[requinix]

15 minutes ago, NotionCommotion said:

Thanks.  What about my ugly getter which is based on naming standards?

I don't care for it because I don't like dynamic getters as a whole, but the design is widespread enough that I'm not going to protest too loudly.

 

15 minutes ago, NotionCommotion said:

  Also thought that the logic for enforcing RankedListInterface for all properties with RankedListAttribute could/should be done elsewhere.

A basic thing like validating an attribute against a property should be done by PHP itself, but since it isn't,

If you're looking for advice there, have RankedListAttribute do its own validation: add an interface (or not) and method for performing validation given a ReflectionProperty, then use your ReflectionAttribute to newInstance() and call it.

[NotionCommotion]

 

1 hour ago, requinix said:

I don't care for it because I don't like dynamic getters as a whole, but the design is widespread enough that I'm not going to protest too loudly.

Is there a way to get away from dynamic getters?  While the following doesn't eliminate them, at least it makes the class that implements RankedListInterface responsible for them.  Maybe better?

interface HasRankedListInterface
{
    public method getProperty(string $propertyName):RankedListInterface;
}

trait RankedListGetterTrait
{
    public method getProperty(string $propertyName):RankedListInterface
    {
        // No validation as a call to a non-RankedListInterface property or undefined property will throw an exception?
        return $this->{'get'.ucfirst($propertyName)}(); // or just $this->$propertyName;
    }
}

Yes I was, thank you, and while not certain, was thinking along those same lines.

1 hour ago, requinix said:

If you're looking for advice there, have RankedListAttribute do its own validation: add an interface (or not) and method for performing validation given a ReflectionProperty, then use your ReflectionAttribute to newInstance() and call it.

[requinix]

1 hour ago, NotionCommotion said:

Is there a way to get away from dynamic getters?  While the following doesn't eliminate them, at least it makes the class that implements RankedListInterface responsible for them.  Maybe better?

Moving the logic to a "getProperty" sweeps the problem under the rug: it's still there, not just as visible.

I think what I'm seeing here that draws my attention most is using properties for half of the work and getter methods for the other half. Pick one: properties (list properties, check attribute, get value) or methods (interface method to list properties with capabilities and possibly getter closures).

[NotionCommotion]

2 hours ago, requinix said:

I think what I'm seeing here that draws my attention most is using properties for half of the work and getter methods for the other half. Pick one: properties (list properties, check attribute, get value)...

That was my original intention, and I thought/hoped that there would be some $propReflect->getValue() method that would work.  And turned out there appeared to be such a method but I "think" they need to be public properties, and was concerned that going down that path would not be good.  I would like to go down this path.  Is there a way to make it work without needing to publicly expose them?

2 hours ago, requinix said:

... or methods (interface method to list properties with capabilities and possibly getter closures).

But it doesn't seem like the cool attribute way of doing things

Your second option inspired me to think of a third potential option.  Put the attributes on the (public) methods instead of the (private/protected) properties.  I haven't looked into it but think it will be viable, however, would still rather put the attributes on the properties if there was a good way to get the values.

[requinix]

46 minutes ago, NotionCommotion said:

That was my original intention, and I thought/hoped that there would be some $propReflect->getValue() method that would work.  And turned out there appeared to be such a method but I "think" they need to be public properties, and was concerned that going down that path would not be good.  I would like to go down this path.  Is there a way to make it work without needing to publicly expose them?

Yes, it's possible, but you're getting to the point where you're abusing reflection so don't.

If making properties public is not acceptable then use methods.

 

46 minutes ago, NotionCommotion said:

But it doesn't seem like the cool attribute way of doing things

The problem isn't the attributes - it's the properties. PHP doesn't have visibility on property reads/writes, so unless you want to switch to an immutable design with readonly properties...

 

46 minutes ago, NotionCommotion said:

Your second option inspired me to think of a third potential option.  Put the attributes on the (public) methods instead of the (private/protected) properties.  I haven't looked into it but think it will be viable, however, would still rather put the attributes on the properties if there was a good way to get the values.

Here's the thought process:

1. Properties are nice, but you need the values from them. Don't want to make them public, shouldn't use reflection (IMO) to bypass visibility.
2. So you can't get the data from the properties. What other mechanisms are available?
3. Get the data from the methods. Dumb little getter methods with no logic.
4. How to get the "ranked list" data?

At this point your answer is attributes. Not unreasonable, but you have to go through reflection and that's a red flag.

5. Mark the methods with attributes to discover them.
6. Still need to get "names" from them, and the methods will be "getName". So you have to unmangle those.

My answer is a regular, everyday interface method.

5. Check that the class implements the proper interface.
6. Call the particular method in the interface that returns a set of keys (names) and values (RankedListInterfaces).

[NotionCommotion]

10 hours ago, requinix said:

Yes, it's possible, but you're getting to the point where you're abusing reflection so don't.

Okay, I won't, but purely out of curiosity,  how would one make it work without needing to publicly expose them or use hackish getters?

10 hours ago, requinix said:

At this point your answer is attributes. Not unreasonable, but you have to go through reflection and that's a red flag.

Thanks, will think twice before considering reflection and not be swayed by all those frameworks that depend on them.

10 hours ago, requinix said:

6. Still need to get "names" from them, and the methods will be "getName". So you have to unmangle those.

Yep, forgot about the dang name.  Don't need it, however, because it is either a ProjectStage or NULL, but still...

10 hours ago, requinix said:

My answer is a regular, everyday interface method.

5. Check that the class implements the proper interface.
6. Call the particular method in the interface that returns a set of keys (names) and values (RankedListInterfaces).

Boring, but simple and easy to maintain. You a proponent of KISS?

[requinix]

3 hours ago, NotionCommotion said:

Okay, I won't, but purely out of curiosity,  how would one make it work without needing to publicly expose them or use hackish getters?

The setAccessible method.

 

3 hours ago, NotionCommotion said:

Boring, but simple and easy to maintain. You a proponent of KISS?

Not really. I'm a proponent of simple when it makes sense, but simple shouldn't be the goal.

[NotionCommotion]

On 11/19/2021 at 7:49 AM, requinix said:

The setAccessible method.

 

Not really. I'm a proponent of simple when it makes sense, but simple shouldn't be the goal.

Sorry, didn't mean to imply that you only provide simple code (which is obvious since you know about that setAccessible black magic!).  I do know a guy, however, that is in charge of what was the largest BSL4 lab at the time, and he definitely believed in KISS.

I ended up going with an interface and it worked perfectly.  Later, a new requirement came up where I needed similar information but before the object had been instantiated.  Given this new requirement, would consider using attributes?  Not positive, but I don't think interfaces support static methods.  If not, under what circumstances would you use attributes?

Thanks

[kicken]

2 hours ago, NotionCommotion said:

Not positive, but I don't think interfaces support static methods.

You can use static methods in an interface.  For example, Symfony's EventSubscriberInterface.

[NotionCommotion]

5 hours ago, kicken said:

You can use static methods in an interface.  For example, Symfony's EventSubscriberInterface.

Thanks Kicken,  I was rather surprised about not being able to do so, and based my understanding on this stackoverflow post and the fact that the PHP interface document doesn't include the word "static" and the PHP static documentation doesn't include the work "interface".  After looking at the post again, I see that the last comment said it was all bunk.