Adding parent:__construct() or not is depends on the use case, so the correct way is to configure it instead of automatically add it as the override part can actually not call parent:__construct() on purpose.

The following code is valid:

class A
{
    public function __construct($a, $b)
    {
        echo 'first';
    }
}

class B extends A
{
    public function __construct()
    {
        echo 'two';
    }
}

ref https://3v4l.org/v1Bvk

You're not wrong, it's valid code yes. However in conjunction with dependency injection I'd probably consider it a code smell myself.

As I say, I'm using it to fix a bug in another rector which makes my code break and thought it might be useful to others.

It's worth noting PHPStan with strict rules reports this as an error:
https://phpstan.org/r/e1284f36-508b-4c79-b275-9e80293b0550

That doesn't mean incorrect, inject or not is always depends, there are many uses in term of containerize object; autowire, initializers, etc, they are there on purpose

I'm not saying it's incorrect to do so?

What I meant is the parent::__construct($arg) is special, the usage can't be judged by automation

PHPStan with strict rules disagrees with you

That's doesn't mean it need to be added, as I said, it always depends on the use case, there are always use case that not call parent is on purpose :)

In which case the claim of "getting to the latest PHPStan level" should probably be removed from this page https://getrector.com/about

As it's its own rule and is not implemented in any rule sets or otherwise enabled by default, I don't understand why this wouldn't be at place being in Rector.

As far as that can't potentially make BC break that cause another reported issue regarding it, and we, as maintainer, need to take care :).

This rule can make more harm than usefull to be honest, yet, you can create your own on your dev for it.

Not sure how a rule which no one has to implement is going to cause BC breaks they're not aware of when they do implement it. Is this the mindset of all maintainers or just yours?

We should probably remove all of these rules as they may cause BC breaks https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#strict

The strict rules mostly about operator changing, which equal the functionality, and configurable to be less strict :)

Changing to add parent::__construct() and its function __construct() param automatically is different, it need to verify:

• how object next be created by service or manually
• what functionality actually happened

that's cannot be applied on file basis, but also on dependencies and config, etc.

configurable to be less strict

Unless I've completely mis-understood how to configure them, the examples for the following all give the same output for the example given regardless of how it's configure. Which would lead me to suggest the configuration is likely pointless and a false-economy:

• BooleanInIfConditionRuleFixerRector
  With true: https://getrector.com/demo/819d0d6e-b0e1-4c1a-9877-c0b2cae1b73f
  With false: https://getrector.com/demo/b3f6ea84-8b80-4782-836c-d7d3c8afa7d7
• BooleanInTernaryOperatorRuleFixerRector
  With true: https://getrector.com/demo/2c92ce14-26b9-4bae-8d07-efec669e94b2
  With false: https://getrector.com/demo/35c872d0-51ad-41d9-930c-a0046916f0fb
• DisallowedEmptyRuleFixerRector
  With true: https://getrector.com/demo/ba09664c-56fd-41ff-b982-83f72e7b09e7
  With false: https://getrector.com/demo/7f1bb92e-5946-4c66-9f86-fe633771f537
• DisallowedShortTernaryRuleFixerRector
  With true: https://getrector.com/demo/b5146212-a39c-45fb-b4bd-78597cbc6773
  With false: https://getrector.com/demo/612108ce-1346-4dac-9972-f801443f24c7

If you like, I can also add a pointless configuration option so it can "be less strict"?

• how object next be created by service or manually
• what functionality actually happened

ContainerGetToConstructorInjectionRector doesn't care about this

Perhaps we should remove that rule also?

I can also move it to the Rector\Strict\Rector if that makes it better?

I see that you reported issue on rector-symfony already for GetToConstructorInjectionRector:

• Add tests for overriding constructors in GetToConstructorInjectionRector rector-symfony#373
• GetToConstructorInjectionRector doesn't implement parent constructor calls properly rector#7814

I think that's should be discussed or fixed there instead of create new rule to cover it.

I did and it would be great if that were to get fixed. It's beyond my knowledge and clearly requires good knowledge of Rector to fix though.

However, this rule (to me) is clearly able to be something in it's own right given that without it manual fixes would be required in order to meet the requirements for PHPStan strict rules:
https://phpstan.org/r/e1284f36-508b-4c79-b275-9e80293b0550

Ok, deal, move to Strict category will be better than 👍

Done 👍

I am not sure about rector workflow error: https://github.com/rectorphp/rector-src/actions/runs/4305120316/jobs/7507111232#step:2:12

Could you rebase? The skip run on fork should work

Oh, I see, looking on older commit before, the repo name check seems still needed

Please rebase to make CI green

Rebased and all green 👍 Thanks for sorting that

Please rebase again to see if CI keep green

Rebased. Looks to be green still and Rector job is skipped. Not sure why the expected checks are showing still. Might be worth trying with another PR to see if it's just this one messing up?

Not sure, could you try create new PR to update workflow config that possibly can solve that to see if it skip on forks? Thank you.

I think this is actually expected :) Rector should run only on our code, as it would force contributors to re-run Rector, which is an obstacle for contributing. Devs should not be forced to put any Rector manuall work to their code.

Rector should only run, when allowed to contribute and handle 100 % work for us and the contributors.

This works as expected 👍

@TomasVotruba no, the pending parts make me cannot merge the PR now

I don't think you'll see the "3 expected checks" going forwards on new PRs. It will just skip the Rector workflow and green tick the PR instead of the pending/running state this one is in. I've seen it before and I think it was resolved by pushing a new commit after the rebase - I'm not sure though but it went away. You can skip the "pending" checks and merge if you have enough privileges.

If I'm wrong it might be branch protection rules and required checks

I don't know, it shows : "Required statuses must pass before merging", that make no longer able to merge PR from forks

@samsonasik I've removed the requirement from the branch protection, as it doesn't make sense to require these if we don't want contributors to mantain Rector changes.

Good to go now 👍

Thank you @jackbentley