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

Mark filters as attributes #93

Open
delolmo opened this issue Dec 22, 2022 · 7 comments
Open

Mark filters as attributes #93

delolmo opened this issue Dec 22, 2022 · 7 comments

Comments

@delolmo
Copy link

delolmo commented Dec 22, 2022

Feature Request

Q A
New Feature yes
RFC no
BC Break no

Summary

Adding the #[Attribute] tag to filters would allow several new use cases for the library.

For example, object validation (i.e., a DTO):

use Laminas\Filter;

final class CreateNewBookDto extends Dto
{
    #[Filter\StringTrim]
    public $author;

    #[Filter\StringTrim]
    #[Filter\StringToUpper]
    public $title;

    public function __construct($author, $title)
    {
         $this->author = $author;
         $this->title = $title;
    }
}

In this case, filters could be read using reflection to create an automated pipeline to filter object properties.

Other cases could include route filtering of HTTP requests, console input filtering, etc.

@Ocramius
Copy link
Member

Sounds like an OK idea, TBH (same for validators).

Note that some of these require external dependencies, and cannot really be safely used as attributes.

@delolmo
Copy link
Author

delolmo commented Dec 23, 2022

Just as a proof of concept, perhaps this could be an interesting implementation:

namespace Laminas\Filter;

use ReflectionAttribute;
use ReflectionClass;

use function class_exists;
use function is_object;

final class AnnotatedObjectFilter implements FilterInterface
{
    public function filter(mixed $object): mixed
    {
        if (! is_object($object)) {
            return $object;
        }

        $className = $object::class;

        if (! class_exists($className)) {
            return $object;
        }

        $reflectionClass = new ReflectionClass($className);

        $properties = $reflectionClass->getProperties();

        foreach ($properties as $property) {
            $attributes = $property->getAttributes(
                name: FilterInterface::class,
                flags: ReflectionAttribute::IS_INSTANCEOF,
            );

            foreach ($attributes as $attribute) {
                $filter = $attribute->newInstance();
                $value  = $filter->filter($property->getValue($object));
                $property->setValue($object, $value);
            }
        }

        return $object;
    }
}

@froschdesign
Copy link
Member

@delolmo
Your implementation ignores external dependencies of filters. The filter plugin manager must be used to retrieve a filter, as the manager resolves the dependencies and also the alias names of filters:

/**
* Plugin manager implementation for the filter chain.
*
* Enforces that filters retrieved are either callbacks or instances of
* FilterInterface. Additionally, it registers a number of default filters
* available, as well as aliases for them.
*
* @final
* @extends AbstractPluginManager<FilterInterface|callable(mixed): mixed>
*/
class FilterPluginManager extends AbstractPluginManager

More on this topic can be found in the documentation of laminas-servicemanager: Plugin Managers

@delolmo
Copy link
Author

delolmo commented Dec 23, 2022

What about something like this then? I am sure there are better ways to do it, just trying to contribute something here. Let me know if there is something else I need to take into account.

use Laminas\ServiceManager\ServiceManager;
use ReflectionAttribute;
use ReflectionClass;

use function is_object;

final class AnnotatedObject implements Filter
{
    /** @var FilterPluginManager|null */
    protected $plugins;

    /**
     * Get plugin manager instance
     *
     * @return FilterPluginManager
     */
    public function getPluginManager()
    {
        $plugins = $this->plugins;
        if (! $plugins instanceof FilterPluginManager) {
            $plugins = new FilterPluginManager(new ServiceManager());
            $this->setPluginManager($plugins);
        }

        return $plugins;
    }

    public function filter(mixed $value): mixed
    {
        if (! is_object($value)) {
            return $value;
        }

        $reflectionClass = new ReflectionClass($value);

        $properties = $reflectionClass->getProperties();

        foreach ($properties as $property) {
            $attributes = $property->getAttributes(
                name: Filter::class,
                flags: ReflectionAttribute::IS_INSTANCEOF,
            );

            foreach ($attributes as $attribute) {
            	$filter = $this->plugin(
            		name: $attribute->getName(), 
            		options: $attribute->getArguments()
        		);
                /** @var mixed $origValue */
                $origValue = $property->getValue($value);
                /** @var mixed $newValue */
                $newValue = $filter->filter($origValue);
                $property->setValue($value, $newValue);
            }
        }

        return $value;
    }

    /**
     * Retrieve a filter plugin by name
     *
     * @param string $name
     * @return FilterInterface|callable(mixed): mixed
     */
    public function plugin($name, array $options = [])
    {
        $plugins = $this->getPluginManager();
        return $plugins->get($name, $options);
    }
    
    /**
     * Set plugin manager instance
     *
     * @return self
     */
    public function setPluginManager(FilterPluginManager $plugins)
    {
        $this->plugins = $plugins;
        return $this;
    }
}

@boesing
Copy link
Member

boesing commented Dec 23, 2022

Maybe we find a way that it is somehow related to laminas-form as well, we do have Filters and Validators there as well and afair there are attributes.

But Overall I like the idea.

@delolmo
Copy link
Author

delolmo commented Dec 2, 2023

I was revisiting this topic and I think the idea still is as an interesting one. Would you like to revisit it?

I have been thinking about what you said about laminas-form, but laminas-filter and laminas-validator should work in a standalone manner, shouldn't they? Perhaps we can move towards implementing the feature in filters and validators and then try to use this new feature in the laminas-form library.

Should I open a PR with my AnnotatedObject implementation?

@boesing
Copy link
Member

boesing commented Dec 4, 2023

I think a PR would be very welcome.
However, you might want to have a look into #118 first and maybe wait some time until we decided on how we want to proceed with this library (esp. the option passing via __construct).

@froschdesign I am not sure if we need the filter plugin manager. Attributes are not annotations which can be invalid.
Not every filter can be an attribute either, so those filters which can be used as attributes are not allowed to have external dependencies anyways, as these cannot be resolved via the attribute notation.

I am not 100% sure if we should actually have this in-place as (at least I) do not see a real use-case for this (besides the very limited functionality provided by @delolmo).
One of my concerns are, that one has to have a mixed property and just by adding attributes, no static analyzer will understand what value is in those properties.

If we provide plugins for analyzers, that might work with analyzers but there is no way of actually verifying that the object has already applied filters or not and thus static analyzers will have a tough time.

That is why I do not use filters outside of forms (in which I can just pass array).
I'd rather have something like:

final class CreateNewBookDto extends Dto
{
    /** 
      * @param non-empty-string $author
      * @param uppercase-string $title
      */
    public function __construct(public readonly string $author, public readonly string $title)
    {
    }
}

With having StringTrim being applied prior instantiating the class.
This provides proper types without having mixed somewhere.

But thats most probably a personal thing and there might be projects and developers who are not that strict when it comes to their types. I would never pass any code review in our company with that but thats just because I have other expectations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants