Skip to content

bug: Fix FullyQualifiedStrictTypesFixer common prefix bug#6898

Merged
Wirone merged 2 commits intoPHP-CS-Fixer:masterfrom
edsrzf:fully-qualified-strict-types-prefix
Apr 20, 2023
Merged

bug: Fix FullyQualifiedStrictTypesFixer common prefix bug#6898
Wirone merged 2 commits intoPHP-CS-Fixer:masterfrom
edsrzf:fully-qualified-strict-types-prefix

Conversation

@edsrzf
Copy link
Copy Markdown
Contributor

@edsrzf edsrzf commented Apr 18, 2023

The fully_qualified_strict_types rule had a bug when a class in the root namespace was used, and that class had a common prefix with the current namespace.

For example, this input:

<?php

namespace Foo;

function foo(\FooBar $v): \FooBar {}

would be transformed into:

<?php

namespace Foo;

function foo(ar $v): ar {}

This change fixes the bug.

@edsrzf edsrzf changed the title Fix FullyQualifiedStrictTypesFixer common prefix bug bug: Fix FullyQualifiedStrictTypesFixer common prefix bug Apr 18, 2023
@Wirone
Copy link
Copy Markdown
Member

Wirone commented Apr 19, 2023

@edsrzf thank you very much for providing fix! Can I ask you to split provided changes into separate commits: one with failing test case, and other one with the fix? That would be easier to verify it locally (by checking out one commit behind). Generally it's good approach to prepare PR with failing test, run the workflow, see it fails and then providing fix, but this unfortunately does not work good for first time contributors, when you need to wait for the approval..

Unfortunately I don't have enough rights to approve workflow, so we need to wait for someone else to trigger it.

@edsrzf edsrzf force-pushed the fully-qualified-strict-types-prefix branch from 1945231 to e2f6244 Compare April 20, 2023 09:34
The fully_qualified_strict_types rule had a bug when a class in the root
namespace was used, and that class had a common prefix with the current
namespace.

For example, this input:

```php
<?php

namespace Foo;

function foo(\FooBar $v): \FooBar {}
```

would be transformed into:

```php
<?php

namespace Foo;

function foo(ar $v): ar {}
```

This change fixes the bug.
@edsrzf
Copy link
Copy Markdown
Contributor Author

edsrzf commented Apr 20, 2023

I've re-pushed my branch with separate commits.

This would be useful information to have in CONTRIBUTING.md. There is a conflicting school of thought that says each commit should pass all tests. I'm happy to follow whatever practices the project prefers.

@Wirone
Copy link
Copy Markdown
Member

Wirone commented Apr 20, 2023

@edsrzf in the end all commits from PR are squashed so in the main branch every commit is passing (in theory 😅). Splitting commits makes sense only for bug fixes - first we make reproduction case and confirm it fails in this particular scenario, then we fix it 🙂.

Copy link
Copy Markdown
Member

@Wirone Wirone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before fix:

vendor/bin/phpunit tests/Fixer/Import/FullyQualifiedStrictTypesFixerTest.php                           
PHPUnit 9.6.6 by Sebastian Bergmann and contributors.

Testing PhpCsFixer\Tests\Fixer\Import\FullyQualifiedStrictTypesFixerTest
.......F.................................                                                                                                                                                      41 / 41 (100%)

Time: 00:00.086, Memory: 24.00 MB

There was 1 failure:

1) PhpCsFixer\Tests\Fixer\Import\FullyQualifiedStrictTypesFixerTest::testNewLogic with data set "common prefix" ('<?php namespace Foo; function...Bar {}', null)
Code build on expected code must not change.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<?php namespace Foo; function foo(\FooBar $v): \FooBar {}'
+'<?php namespace Foo; function foo(ar $v): ar {}'

LGTM 👍

@Wirone Wirone added the RTM Ready To Merge label Apr 20, 2023
@Wirone Wirone merged commit db385b9 into PHP-CS-Fixer:master Apr 20, 2023
@Wirone
Copy link
Copy Markdown
Member

Wirone commented Apr 20, 2023

Thank you, @edsrzf 🍻

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