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

[BrowserKit] Fixed BC-break introduced by rename of Client to Browser #31040

Merged
merged 1 commit into from Apr 15, 2019

Conversation

Projects
None yet
4 participants
@Devristo
Copy link
Contributor

commented Apr 9, 2019

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #31039
License MIT
Doc PR

Since #30541 the inheritance hierarchy of \Symfony\Component\BrowserKit\Client has changed. Notably the test.client no longer is an instance of \Symfony\Component\BrowserKit\Client.

This PR uses class_alias to fix the class hierarchy similarly as has been done in Twig. In this case I copied the approach of Twig_TokenParser_AutoEscape and \Twig\TokenParser\AutoEscapeTokenParser

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

class_alias() doesn't work really well for deprecation layers because the notice is thrown depending on the loading order. Deprecating the service in favor of test.browser might be better?

@Devristo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

It is true that class_alias is limited. However I am not sure if I understand your suggestion. The library I am using has the following class definition:

<?php

declare(strict_types=1);

namespace FriendsOfBehat\SymfonyExtension\Driver;

use Behat\Mink\Driver\BrowserKitDriver;
use Symfony\Component\BrowserKit\Client;
use Symfony\Component\HttpKernel\KernelInterface;

final class SymfonyDriver extends BrowserKitDriver
{
    public function __construct(KernelInterface $kernel, string $baseUrl)
    {
        if (!$kernel->getContainer()->has('test.client')) {
            throw new \RuntimeException(sprintf(
                'Kernel "%s" used by Behat with "%s" environment and debug %s does not have "test.client" service. ' . "\n" .
                'Please make sure the kernel is using "test" environment or have "framework.test" configuration option enabled.',
                get_class($kernel),
                $kernel->getEnvironment(),
                $kernel->isDebug() ? 'enabled' : 'disabled'
            ));
        }

        $testClient = $kernel->getContainer()->get('test.client');

        if (!$testClient instanceof Client) {
            throw new \RuntimeException(sprintf(
                'Service "test.client" should be an instance of "%s", "%s" given.',
                Client::class,
                get_class($testClient)
            ));
        }

        parent::__construct($testClient, $baseUrl);
    }
}

The service test.client could be deprecated. But still the instanceof check should succeed. I don't see how to do the latter without class_alias (or rearranging the class hierarchy).

Or do you mean that in addition to the class_alias to define a new service test.browser and deprecating test.client?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

I was hopping that creating a new service would be enough but it wouldn't allow passing legacy type hints.
The best solution then is to make AbstractBrowser extend from Client.

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 9, 2019

@Devristo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

If I am not mistaken, the Client should also have the same public interface as AbstractBrowser then. I'm not sure if that's worth it? Wouldn't it be against the intent of the original PR?

It is possible though, rename AbstractBrowser back to Client. Create a new empty AbstractBrowser extending from Client. Then upon removal of Client drop AbstractBrowser and rename Client to AbstractBrowser. Bit cumbersome, but perhaps the safest way.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

It does have the same public interface, see its implementation, unless I missed something?

@Devristo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

I meant that all methods from AbstractBrowser have to be moved to Client if Client is the top of the hierarchy.

Anyway, I am willing to do this. However I am not sure how deprecation will work. We cannot trigger an error in Client then as AbstractBrowser will depend on it?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

We will have to remove the deprecation notice and keep the annotation. DebugClassLoader will still warn ppl that extend Client directly.

@Devristo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Sounds good. I think I have one last question concerning the deprecation messages. Currently the tests expect them on AbstractBrowser methods.

Calling the "Symfony\Component\BrowserKit\AbstractBrowser::getInternalRequest()" method before the "request()" one is deprecated since Symfony 4.1 and will throw an exception in 5.0.

If I alter the class hierarchy __METHOD__ will resolve to methods on Client and yield this:

 Calling the "Symfony\Component\BrowserKit\Client::getInternalRequest()" method before the "request()" one is deprecated since Symfony 4.1 and will throw an exception in 5.0.

I can do three things:

  1. Alter the tests to expect the latter deprecation instead of the first. But people might be confused to see deprecated Client appear the message while they don't depend on it themselves.
  2. Hardcode the deprecation messages such that they refer to AbstractBrowser instead.
  3. Use static::class or get_called_class to generate deprecation messages. This means that the message contains always the concrete instance type. For example KernelBrowser::getInternalRequest().

I don't know how you guys usually solve this. I don't see evidence of variant 2 or 3 in the code base so far. Solution 1 is the way to go?

Thanks for bearing with me and my questions 👍

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Option 1. makes the most sense to me: all existing code uses Client - and not yet written code should not use the method anyway.

@Devristo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

You are right :). I have pushed a separate commit with the changes we discussed.

@fabpot

fabpot approved these changes Apr 15, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Thank you @Devristo.

@fabpot fabpot merged commit 6a94dea into symfony:master Apr 15, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 15, 2019

bug #31040 [BrowserKit] Fixed BC-break introduced by rename of Client…
… to Browser (Devristo)

This PR was squashed before being merged into the 4.3-dev branch (closes #31040).

Discussion
----------

[BrowserKit] Fixed BC-break introduced by rename of Client to Browser

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #31039
| License       | MIT
| Doc PR        |

Since #30541 the inheritance hierarchy of `\Symfony\Component\BrowserKit\Client` has changed. Notably the test.client no longer is an instance of `\Symfony\Component\BrowserKit\Client`.

This PR uses `class_alias` to fix the class hierarchy similarly as has been done in Twig. In this case I copied the approach of `Twig_TokenParser_AutoEscape` and `\Twig\TokenParser\AutoEscapeTokenParser`

Commits
-------

6a94dea [BrowserKit] Fixed BC-break introduced by rename of Client to Browser
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.