Add foreach processor

[ThibautSF]

Add Foreach processor (and test files)

Complete list in #1812

[ThibautSF]

Note: I will do some other processor in the list, only did one for now because want to check :

• if the procedure is ok (nothing is missing)
• check the list, if everything can be added

[deguif]

As field and processor are required as stated in the table options of the official documentation (official doc) I would make them parameters to the constructor to avoid constructing an invalid foreach processor. Is there any reason it can't be done this way?
I see your comment about setRawProcessor but I'm not sure to understand why we need this.

Wouldn't a setProcessor(new Remove('user_agent')) or new ForeachProcessor('field', new Remove('user_agent')) work as expected (to use the example you put in annotation of the setRawProcessor()method)?

[ThibautSF]

As field and processor are required as stated in the table options of the official documentation (official doc) I would make them parameters to the constructor to avoid constructing an invalid foreach processor. Is there any reason it can't be done this way?

PHP is one of the few languages which doesn't allow the constructor overloading... And I often forgot that.
My initial idea was to use 2 constructors :

• __construct(string $field, AbstractProcessor $processor)
• __construct(string $field, array $rawprocessor)

Similar behaviour can be made with public static class and private constructor: but thus mean that this class won't be instantiated like the other processors. Might be a bit strange as final user?

I see your comment about setRawProcessor but I'm not sure to understand why we need this.

The setRawProcessor is needed to easily add a processor which doesn't exist as an Elastica class.

Wouldn't a setProcessor(new Remove('user_agent')) or new ForeachProcessor('field', new Remove('user_agent')) work as expected (to use the example you put in annotation of the setRawProcessor()method)?

Yes of course.

But while I'm thinking about it, the most frequent instance will be with AbstractProcessor.
A thing that can be done is :

     /**
     * Foreach constructor.
     */
    public function __construct(string $field, AbstractProcessor $processor)
    {
        $this->setField($field);
        $this->setProcessor($processor);
    }

Thus if the user wants to use a custom processor:

$processor = ForeachProcessor('field', null);
$processor->setRawProcessor(['remove' => ['field' => 'user_agent']]);

And in other case:

$processor = ForeachProcessor('field', new Remove('user_agent'));

[deguif]

@ThibautSF I better understand now with your explanation.

We can kind of simulate use of constructor overloading with:

/**
 * @param AbstractProcessor|array $processor
 */
public function __construct(string $field, $processor)
{
    $this->setField($field);
    
    if ($processor instanceof AbstractProcessor) {
        $this->setProcessor($processor);
    } elseif (\is_array($processor)) {
        $this->setRawProcessor($processor);
    } else {
        throw new \TypeError('Expected xxx');
    }
}

That can later be replaced with native union type when dropping support for PHP versions < 8.0

Not sure if we have to support the raw processor format, but @ruflin will be of better help for this.

[ThibautSF]

I was working on

    /**
     * Foreach constructor.
     *
     * @param AbstractProcessor $processor
     */
    public function __construct(string $field, ?AbstractProcessor $processor)
    {
        $this->setField($field);
        $this->setProcessor($processor);
    }

But your solution looks better.

[ruflin]

Sorry, a bit late to comment but I'm curious what we need to move this forward? @ThibautSF @deguif ?

[ThibautSF]

Should be OK

Just, code will need to be a bit updated if there is any modification due to #1824

[ruflin]

Sounds like we should resolve #1824 then. @deguif WDYT?

[deguif]

@ThibautSF can you rebase the PR. Overriding _getBaseName() is not needed anymore as the base class will do the job now.

[ThibautSF]

First time rebasing a PR : Followed this guide https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

[deguif]

It seems rebase has not worked as it should as there are lot of commit not introduced in this PR.
Would you mind allowing maintainers to push to your branch ?

[ThibautSF]

Should be already allowed.

EDIT:
And the ThibautSF:add-foreach-processor branch is ahead of ruflin:master.
master...ThibautSF:add-foreach-processor

[deguif]

@ThibautSF should be good now.

[ruflin]

Merging this even though I assume we didn't really align yet on #1824 ;-) But we should not block contributions based on this discussion and we can add an alias at any time.

[deguif]

This one shouldn't be impacted as it conflicts with reserved keywords and will need a Processor suffix ;)