Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[BrowserKit] Rename Client to Browser#30541
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
b5151a7 to3a84669CompareThis PR was merged into the 4.3-dev branch.Discussion----------[BrowserKit] Rename Client to Browser| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | yes| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | n/a| License | MIT| Doc PR | n/a`Client` is very generic and used in 3 places: BrowserKit, HttpKernel, and FramewrokBundle. Each Client extends another one. So, to make things clearer, I'd like to rename Client to Browser like this:Symfony\Component\BrowerKit\Client -> AbstractBrowserSymfony\Component\HttpKernel\Client -> HttpKernelBrowserSymfony\Bundle\FrameworkBundle\Client -> KernelBrowserThe next PR will introduce an `HttpBrowser` based on the new HttpClient component :)Commits-------dbe4f86 renamed Client to Browser
| * @returnClient AClient instance | ||
| * @returnKernelBrowser AKernelBrowser instance | ||
| */ | ||
| protectedstaticfunctioncreateClient(array$options = [],array$server = []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should we rename that method too ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I thought about renaming methods but I think that's too much. What others think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
With a hard copy pasting you could also add return type hints on methods.
stof commentedMar 13, 2019
This is a BC break, because the BC class |
stof commentedMar 13, 2019
I think you will need to use class aliases here instead., to preserve the class hierarchy. |
Devristo commentedApr 9, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yes, it breaks at leasthttps://github.com/FriendsOfBehat/SymfonyExtension andhttps://github.com/minkphp/MinkBrowserKitDriver. For now people can workaround it by adding the following to their test bootstrap: This should be near the top, as it will only work when the class has not yet been autoloaded. |
fabpot commentedApr 9, 2019
@Devristo Can you create a new issue... and if you can provide a PR, that would be wonderful :) |
… 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
…icolas-grekas)This PR was merged into the 4.3 branch.Discussion----------[FramworkBundle][HttpKernel] fix KernelBrowser BC layer| Q | A| ------------- | ---| Branch? | 4.3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#31762| License | MIT| Doc PR | -Swap the order of inheritance to preserve BC with legacy `Client` type hints.From#30541Commits-------1a4c254 [FramworkBundle][HttpKernel] fix KernelBrowser BC layer
…bstractBrowser since SF 4.3 (Calex92)This PR was merged into the 4.4 branch.Discussion----------[Browserkit] The Client class has been replaced by the AbstractBrowser since SF 4.3I noticed that in the BrowserKit documentation, there was multiple references to the old ``Symfony\Component\BrowserKit\Client`` class instead of the ``Symfony\Component\BrowserKit\AbstractBrowser`` one.See [https://github.com/symfony/symfony/pull/30541](https://github.com/symfony/symfony/pull/30541)There was a previous merge request for SF5 here (#12717), but I don't know if it was possible for me to use it since I'm quite new with Github best practice about contribution.Don't hesitate to tell me if I'm wrong!Commits-------a3ff4e0 The Client class has been replaced by the AbstractBrowser since SF 4.3
Clientis very generic and used in 3 places: BrowserKit, HttpKernel, and FramewrokBundle. Each Client extends another one. So, to make things clearer, I'd like to rename Client to Browser like this:Symfony\Component\BrowerKit\Client -> AbstractBrowser
Symfony\Component\HttpKernel\Client -> HttpKernelBrowser
Symfony\Bundle\FrameworkBundle\Client -> KernelBrowser
The next PR will introduce an
HttpBrowserbased on the new HttpClient component :)