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

Move declaration of replacements and selectors to new function #815

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrewnicols
Copy link
Contributor

@andrewnicols andrewnicols commented Oct 14, 2021

Move declaration of replacements and selectors to new function

At present the declaration of all raw un-processed selectors and their
replacements is in either the class body (private vars) or the
constructor.

This has the effect of modifying the original selectors on the class
instance vars during class instantiation, and also prevents any subclass
of ExactNamedSelector or PartialNamedSelector from making changes to any
replacement that has been explicitly overridden in those classes
constructors.

For example, it is impossible for a child class of either of these to
further override the '%tagTextMatch%' replacement because those changes
would either be overridden in the constructor of its parent, or all
original selector replacements would not be applied.

class MyPartialNamedSelector extends PartialNamedSelector
{

    public function __construct()
    {
        // Note: Calling the constructor here (before registerReplacement)
        // will mean that the call to registerReplacement will update the
        // replacement, but only for any new selector defined after the
        // replacement is defined.
        // It will not have any effect upon any selector or replacement
        // already registered.
        parent::__construct();

        $this->registerReplacement('%tagTextMatch%', 'contains(text(), %locator%)');

        // Note: Calling the constructor here (after registerReplacement) will
        // cause the above tagTextMatch replacement to be overwritten by the
        // override defined in the constructor of PartialNamedSelector.
        parent::__construct();
    }
}

Furthermore the original values of both the selectors and replacements
have been mutated in the constructor so it is not possible for any child
class to simply re-apply the original translations.

The only solution to this problem is to extend the NamedSelector base
class and manually copy the standard replacements from
PartialNamedSelector or ExactNamedSelector (as appropriate) into your
new child class, but this approach is not sustainable.

This patch modifies the Selector classes to:

  • moves the storage and fetching of the original, unmodified and
    untranslated selectors and replacements to a function. This
    effectively makes them immutable; and
  • allows the child classes to override or define their own selectors
    and replacements by simply overriding the parent class function and
    updating or adding the required array values.

This change allows the replacements (or selectors) to be updated as in
the following example:

class MyPartialNamedSelector extends PartialNamedSelector
{
    protected function getRawReplacements()
    {
        return array_merge(parent::getRawReplacements(), [
            '%tagTextMatch%' => 'contains(text(), %locator%)',
        ]);
    }

    protected function getRawSelectors()
    {
        return array_merge(parent::getRawSelectors(), [
            'link' => './/a[./@href][%tagTextMatch%]',
        ]);
    }
}

Note: For any usage where the child class was directly extending the
NamedSelector class and defining its custom selectors and replacements
in the constructor, these will need to be updated to define the relevant
getRawSelectors() and/or getRawReplacements() classes as above.

This change should be considered a possibly breaking change in this
situation and therefore would demand a new major release (1.10.0
presumably).

Whilst this is a breaking change, it will have provide a more
sustainable approach in future for more advanced uses of Mink.

@andrewnicols
Copy link
Contributor Author

Note: I have tried to determine how a unit test could be written here, but cannot think of a good way which does not use either a set fixture, or eval() to define a new class. If either of tese is appropriate then I can look to write some unit tests for it.

At present the declaration of all raw un-processed selectors and their
replacements is in either the class body (private vars) or the
constructor.

This has the effect of modifying the original selectors on the class
instance vars during class instantiation, and also prevents any subclass
of ExactNamedSelector or PartialNamedSelector from making changes to any
replacement that has been explicitly overridden in those classes
constructors.

For example, it is impossible for a child class of either of these to
further override the '%tagTextMatch%' replacement because those changes
would either be overridden in the constructor of its parent, or all
original selector replacements would not be applied.

```
class MyPartialNamedSelector extends PartialNamedSelector
{

    public function __construct()
    {
        // Note: Calling the constructor here (before registerReplacement)
        // will mean that the call to registerReplacement will update the
        // replacement, but only for any new selector defined after the
        // replacement is defined.
        // It will not have any effect upon any selector or replacement
        // already registered.
        parent::__construct();

        $this->registerReplacement('%tagTextMatch%', 'contains(text(), %locator%)');

        // Note: Calling the constructor here (after registerReplacement) will
        // cause the above tagTextMatch replacement to be overwritten by the
        // override defined in the constructor of PartialNamedSelector.
        parent::__construct();
    }
}
```

Furthermore the original values of both the selectors and replacements
have been mutated in the constructor so it is not possible for any child
class to simply re-apply the original translations.

The only solution to this problem is to extend the NamedSelector base
class and manually copy the standard replacements from
PartialNamedSelector or ExactNamedSelector (as appropriate) into your
new child class, but this approach is not sustainable.

This patch modifies the Selector classes to:
* moves the storage and fetching of the original, unmodified and
  untranslated selectors and replacements to a function. This
  effectively makes them immutable; and
* allows the child classes to override or define their own selectors
  and replacements by simply overriding the parent class function and
  updating or adding the required array values.

This change allows the replacements (or selectors) to be updated as in
the following example:
```
class MyPartialNamedSelector extends PartialNamedSelector
{
    protected function getRawReplacements()
    {
        return array_merge(parent::getRawReplacements(), [
            '%tagTextMatch%' => 'contains(text(), %locator%)',
        ]);
    }

    protected function getRawSelectors()
    {
        return array_merge(parent::getRawSelectors(), [
            'link' => './/a[./@href][%tagTextMatch%]',
        ]);
    }
}
```

Note: For any usage where the child class was directly extending the
`NamedSelector` class and defining its custom selectors and replacements
in the constructor, these will need to be updated to define the relevant
`getRawSelectors()` and/or `getRawReplacements()` classes as above.

This change should be considered a possibly breaking change in this
situation and therefore would demand a new major release (1.10.0
presumably).

Whilst this is a breaking change, it will have provide a more
sustainable approach in future for more advanced uses of Mink.
@aik099
Copy link
Member

aik099 commented Oct 14, 2021

Note: I have tried to determine how a unit test could be written here, but cannot think of a good way which does not use either a set fixture, or eval() to define a new class. If either of tese is appropriate then I can look to write some unit tests for it.

@andrewnicols , you can test this by defining a new selector sub-class with replacements, that:

  1. enhance existing ones to make them find more stuff
  2. add new replacements that are checked via new tests

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

Successfully merging this pull request may close these issues.

2 participants