Skip to content

Symfony42: Remove invalid rectors#1627

Closed
bendavies wants to merge 1 commit intorectorphp:masterfrom
bendavies:patch-2
Closed

Symfony42: Remove invalid rectors#1627
bendavies wants to merge 1 commit intorectorphp:masterfrom
bendavies:patch-2

Conversation

@bendavies
Copy link
Copy Markdown
Contributor

Ref: symfony/symfony@f75fffa

These deprecation warnings are for when inherited class calls parent method.

Ref: symfony/symfony@f75fffa 

These deprecation warnings are for when inherited class calls parent method.
@TomasVotruba
Copy link
Copy Markdown
Member

TomasVotruba commented Jun 21, 2019

I don't understand. The commit link is confusing.
Could you explain bit more?

@bendavies
Copy link
Copy Markdown
Contributor Author

bendavies commented Jun 21, 2019

These deprecations warnings for for classes extending the symfony ones. for example.

This will trigger a deprecation warning.

<?php

class ClassThatInheritClient extends Client
{
    public function submit(DomCrawlerForm $form, array $values []))
    {
        return parent::submit($form, $values);
    }
}

It should be fixed to:

<?php

class ClassThatInheritClient extends Client
{
    public function submit(DomCrawlerForm $form, array $values [], array $serverParameters = [])))
    {
        return parent::submit($form, $values, $serverParameters);
    }
}

The current rectors are incorrrectly changing calls to the current symfony methods, i.e.
Changing this:

<?php


class MyService
{
    public function __invoke()
    {
        $client = new \Symfony\Component\HttpKernel\Client(...);
        $client->submit($form);
    }
}

to this

<?php


class MyService
{
    public function __invoke()
    {
        $client = new \Symfony\Component\HttpKernel\Client(...);
        $client->submit($form, []);
    }
}

@TomasVotruba
Copy link
Copy Markdown
Member

TomasVotruba commented Jun 23, 2019

I see. In that way it should not be removed but fixed.

There should be new configuration option, that adds it only to parent call + class method.

 services:
     Rector\Rector\Argument\ArgumentAdderRector:
         Symfony\Component\BrowserKit\Client:
             submit:
                 2:
                     name: 'serverParameters'
                     default_value: []
+                    type: ['class_method', 'parent_call']

@bendavies
Copy link
Copy Markdown
Contributor Author

👍 so it's not possible right now?

@TomasVotruba
Copy link
Copy Markdown
Member

Not yet

@bendavies
Copy link
Copy Markdown
Contributor Author

Right. So shall we merge this for now? As this config is causing incorrect changes

@TomasVotruba
Copy link
Copy Markdown
Member

No, it needs to be fixed so it works as it should.
Merging would cause other troubles with missing migration

@bendavies
Copy link
Copy Markdown
Contributor Author

up to you, but at the moment keeping this causes an incorrect migration, which is better than no migration.

@TomasVotruba
Copy link
Copy Markdown
Member

TomasVotruba commented Jun 28, 2019

I know. But finished work is better than double work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants