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

Particular use question :) #4

Closed
AlvaroRosado opened this issue Feb 29, 2024 · 7 comments
Closed

Particular use question :) #4

AlvaroRosado opened this issue Feb 29, 2024 · 7 comments

Comments

@AlvaroRosado
Copy link

Hi!, congratulations on the work, I love the idea and the use of the package, it's something I've implemented by hand before.

I have some questions for a particular use case, let's see if you can help me.

I'm defining my own collections to ensure type safety.

I have something similar to this =>

StudentCollection extends EntityCollection and the latter lives in my Shared folder and this last is extending some classes from this package.

I would like to achieve the same functionality as with the LazyMatchingCollection, chaining the matchings but within my EntityCollection itself.

Something like this: StundetCollection()->enabled()->theLastMonth();

In addition to this, it would be ideal for me if the matching method itself is only accessible from within StundentCollection but not exposed as public.

I have done some tests and I can only protect the matching method by extending my EntityCollection from AbstractCollectionDecorator.

And similarly, I only achieve the chaining of matching if I don't use my own collection and in the entity I directly use LazyMatchingCollection.

In summary, I would like to achieve both behaviors in my base collection so that I can extend them to the rest of my collections.

Thank you very much in advance!

@priyadi
Copy link
Member

priyadi commented Feb 29, 2024

Can you post a code sample? I think what you want is doable. It is only a matter if it is possible to use a ready-made class from this project, or if you need to copy & paste relevant code.

@AlvaroRosado
Copy link
Author

AlvaroRosado commented Mar 1, 2024

Can you post a code sample? I think what you want is doable. It is only a matter if it is possible to use a ready-made class from this project, or if you need to copy & paste relevant code.

Of course!.

This is my EntityCollection (aka BaseClass)

class EntityCollection extends AbstractCollectionDecorator
{
    /**
     * @param Collection&Selectable $collection
     */
    public function __construct(private Collection $collection)
    {
        if (!$collection instanceof Selectable) {
            throw new \RuntimeException('The wrapped collection must implement the Selectable interface.');
        }
    }


    protected function getWrapped(): Collection
    {
        return $this->collection;
    }
}

This is my custom collection


class LClassStudentCollection extends EntityCollection
{
    public function enabled(): ?LClassStudentCollection
    {
        $criteria = Criteria::create()->where(Criteria::expr()->eq('status', LClassStudentStatus::ACCEPTED));
        /** @var LClassStudent $student */
        return new LClassStudentCollection($this->getWrapped()->matching($criteria));
    }

    public function inLastMonth(): ?LClassStudentCollection
    {
        $lastMonth = new \DateTime('first day of last month');
        $criteria = Criteria::create()->where(Criteria::expr()->gt('createdAt', $lastMonth));
        /** @var LClassStudent $student */
        return new LClassStudentCollection($this->getWrapped()->matching($criteria));
    }

}

In this way, it works perfectly, but I don't have the feature of chaining matching methods.

I haven't managed to make the matching work for me. I've tried copying the content of the LazyMatchingReadableCollection class to my EntityCollection and extending it from SelectableReadableCollectionDecorator, but it doesn't work; it gives me a copy error.

For example:

class EntityCollection extends SelectableReadableCollectionDecorator
{
    protected $resultCache = null;
    protected ?Criteria $criteria = null;

    /**
     * @param ReadableCollection<TKey,T> $collection
     */
    public function __construct(
        ReadableCollection $collection
    ) {
        if (!$collection instanceof Selectable) {
            throw new \InvalidArgumentException('Collection must be selectable');
        }

        parent::__construct($collection);
    }

    public function __clone()
    {
        /** @var ?Criteria */
        $copy = deep_copy($this->criteria);

        $this->criteria = $copy;
        $this->resultCache = null;
    }

    /**
     * @return ReadableCollection<TKey,T>&Selectable<TKey,T>
     */
    protected function getWrapped(): ReadableCollection&Selectable
    {
        if ($this->criteria === null) {
            return parent::getWrapped();
        } elseif ($this->resultCache !== null) {
            return $this->resultCache;
        } else {
            return $this->resultCache = parent::getWrapped()->matching($this->criteria);
        }
    }

    public function matching(Criteria $criteria): EntityCollection
    {
        $clone = clone $this;

        if ($clone->criteria === null) {
            $clone->criteria = $criteria;
        } else {
            $clone->criteria = CriteriaUtil::mergeCriteria($clone->criteria, $criteria);
        }

        return $clone;
    }
}

class LClassStudentCollection extends EntityCollection
{
    public function enabled(): ?LClassStudentCollection
    {
        $criteria = Criteria::create()->where(Criteria::expr()->eq('status.value', LClassStudentStatus::ACCEPTED));
        /** @var LClassStudent $student */
        return new LClassStudentCollection($this->matching($criteria));
    }

    public function inLastMonth(): ?LClassStudentCollection
    {
        $lastMonth = new \DateTime('first day of last month');
        $criteria = Criteria::create()->where(Criteria::expr()->gt('createdAt', $lastMonth));
        /** @var LClassStudent $student */
        return new LClassStudentCollection($this->matching($criteria));
    }

}

Using this in this way:

 protected function execute(InputInterface $input, OutputInterface $output): int
    {
        $io = new SymfonyStyle($input, $output);
        $class = $this->classRepository->findById(Uuid::fromString('ffb066a1-08bc-4021-95d9-c43848ae1ff1'));

        /** @var LClassStudent $student */
        foreach ($class->students()->enabled()->inLastMonth() as $student) {
            $io->info((string) $student->id());
        }

        return Command::SUCCESS;
    }

Give me this error when InLastMonth() method is called

In DeepCopy.php line 262:
                                                                                  
  Cannot modify readonly property Doctrine\Common\Collections\Expr\Value::$value  

I have tried some more tests, but I don't see a way to get both of them to work together. Additionally, it would be ideal for me if the matching method does not remain public.

@AlvaroRosado
Copy link
Author

AlvaroRosado commented Mar 4, 2024

I'm still investigating my error.
It seems that there's already an issue reported in the deep_copy function

myclabs/DeepCopy#174

)Doctrine\Common\Collections\Expr class in Doctrine has readonly properties in the constructor

@AlvaroRosado
Copy link
Author

Just to provide more information, I have managed to make the method chaining work if I don't use deep_copy and only use clone methods. However, obviously now I cannot declare matching as protected due to the interfaces.

class EntityCollection extends SelectableReadableCollectionDecorator
{
    protected $resultCache = null;
    protected ?Criteria $criteria = null;

    public function __construct(
        ReadableCollection $collection
    ) {
        parent::__construct($collection);
    }

    public function __clone()
    {
        /** @var ?Criteria */
        $copy = $this->criteria ? clone $this->criteria : null;

        $this->criteria = $copy;
        $this->resultCache = null;
    }

    protected function getWrapped(): ReadableCollection&Selectable
    {
        if ($this->criteria === null) {
            return parent::getWrapped();
        } elseif ($this->resultCache !== null) {
            return $this->resultCache;
        } else {
            return $this->resultCache = parent::getWrapped()->matching($this->criteria);
        }
    }

    public static function mergeCriteria(Criteria $criteria1, Criteria $criteria2): Criteria
    {
        /** @var Criteria */
        $criteria = clone $criteria1;

        if ($criteria2->getFirstResult() !== null) {
            $criteria->setFirstResult($criteria2->getFirstResult());
        }

        if ($criteria2->getMaxResults() !== null) {
            $criteria->setMaxResults($criteria2->getMaxResults());
        }

        $where = $criteria2->getWhereExpression();
        if ($where !== null) {
            $criteria->andWhere($where);
        }

        $criteria->orderBy([
            ...$criteria->getOrderings(),
            ...$criteria2->getOrderings(),
        ]);

        return $criteria;
    }


    public function matching(Criteria $criteria): EntityCollection
    {
        $clone = clone $this;

        if ($clone->criteria === null) {
            $clone->criteria = $criteria;
        } else {
            $clone->criteria = self::mergeCriteria($clone->criteria, $criteria);
        }

        return $clone;
    }
}

@priyadi
Copy link
Member

priyadi commented Mar 5, 2024

Thank you for your investigation. I didn't encounter the error because so far I only use it for straight ->eq() matchings. I'll see what I can do.

@priyadi
Copy link
Member

priyadi commented Mar 5, 2024

2.1.4 should fix the deep_copy problem.

Regarding your original question, can you just stack the decorators?

class EntityCollection extends AbstractCollectionDecorator
{
    private Collection $collection;

    /**
     * @param Collection&Selectable $collection
     */
    public function __construct(Collection $collection)
    {
        if (!$collection instanceof Selectable) {
            throw new \RuntimeException('The wrapped collection must implement the Selectable interface.');
        }

        $this->collection = new LazyMatchingCollection($collection);
    }


    protected function getWrapped(): Collection
    {
        return $this->collection;
    }
}

@AlvaroRosado
Copy link
Author

AlvaroRosado commented Mar 5, 2024

2.1.4 should fix the deep_copy problem.
Regarding your original question, can you just stack the decorators?

class EntityCollection extends AbstractCollectionDecorator
{
    private Collection $collection;

    /**
     * @param Collection&Selectable $collection
     */
    public function __construct(Collection $collection)
    {
        if (!$collection instanceof Selectable) {
            throw new \RuntimeException('The wrapped collection must implement the Selectable interface.');
        }

        $this->collection = new LazyMatchingCollection($collection);
    }


    protected function getWrapped(): Collection
    {
        return $this->collection;
    }
}

Hi!. thanks deep_copy bug is fixed for me in the last release!.

Regarding the solution for my case that you propose, it doesn't work because if I make a return new LClassStudentCollection($this->getWrapped()->matching($criteria)); from any of the children of EntityCollection,

That matching return type is of type ReadableCollection&Selectable and not of type Collection like the constructor.

I think my case is very specific and I don't believe it happens to many people, so you can consider the issue closed.

In the end, I've made EntityCollection implement Collection and I've written custom code to make my case work.

Many thanks for your time and assistance!

@priyadi priyadi closed this as completed Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants