Skip to content

docs: [4.4] fix Rector's incorrect refactoring - #7650

Merged
samsonasik merged 1 commit into
codeigniter4:4.4from
kenjis:skip-rector-AutoRouterImproved
Jul 6, 2023
Merged

samsonasik merged 1 commit into
codeigniter4:4.4from
kenjis:skip-rector-AutoRouterImproved

Conversation

@kenjis

@kenjis kenjis commented Jul 3, 2023

Copy link
Copy Markdown
Member

Description
Related #7649

The following refactoring will break the code.

Two tests will fail:

  • AutoRouterImprovedTest::testFindsControllerAndMethodAndParam()
  • AutoRouterImprovedTest::testPermitsURIWithUnderscoreParam()
1) system/Router/AutoRouterImproved.php:294

    ---------- begin diff ----------
@@ @@

             // Update the positions.
             $this->methodPos = $this->paramPos;
-            if ($params === []) {
-                $this->paramPos = null;
-            }
+            $this->paramPos = null;
             if ($this->paramPos !== null) {
                 $this->paramPos++;
             }
    ----------- end diff -----------

Applied rules:
 * RemoveAlwaysTrueIfConditionRector

https://github.com/codeigniter4/CodeIgniter4/actions/runs/5449192482/jobs/9913186865

Checklist:

  • Securely signed commits
  • [] Component(s) with PHPDoc blocks, only if necessary or adds value
  • [] Unit testing, with >80% coverage
  • [] User guide updated
  • Conforms to style guide

@kenjis kenjis added the 4.4 label Jul 3, 2023
@samsonasik

Copy link
Copy Markdown
Member

could you create a simple reproducible bug on https://getrector.com/demo , it seems correct just by looking at it, but probably it prefilled/unset in other side that cause side effect on the property value.

@kenjis

kenjis commented Jul 5, 2023

Copy link
Copy Markdown
Member Author

@samsonasik Created rectorphp/rector#8039

@samsonasik

Copy link
Copy Markdown
Member

@kenjis the @var seems can be used for it, see https://getrector.com/demo/93fe0739-0ee0-4587-9456-1c6901384da8

@kenjis
kenjis force-pushed the skip-rector-AutoRouterImproved branch from 60f82bf to 69f1aba Compare July 6, 2023 04:25
@kenjis

kenjis commented Jul 6, 2023

Copy link
Copy Markdown
Member Author

@samsonasik Thanks. I removed skipping RemoveAlwaysTrueIfConditionRector,
and added @phpstan-var.

@kenjis kenjis changed the title chore: skip RemoveAlwaysTrueIfConditionRector for AutoRouterImproved docs: fix Rector's incorrect refactoring Jul 6, 2023
@kenjis kenjis changed the title docs: fix Rector's incorrect refactoring docs: [4.4] fix Rector's incorrect refactoring Jul 6, 2023
@kenjis

kenjis commented Jul 6, 2023

Copy link
Copy Markdown
Member Author

The following error will be fixed by #7661

1) system/Commands/Utilities/Routes.php:118

    ---------- begin diff ----------
@@ @@
         $definedRouteCollector = new DefinedRouteCollector($collection);

         foreach ($definedRouteCollector->collect() as $route) {
-            if (is_string($route['handler']) || $route['handler'] instanceof Closure) {
-                $sampleUri = $uriGenerator->get($route['route']);
-                $filters   = $filterCollector->get($route['method'], $sampleUri);
-
-                $routeName = ($route['route'] === $route['name']) ? '»' : $route['name'];
-
-                $tbody[] = [
-                    strtoupper($route['method']),
-                    $route['route'],
-                    $routeName,
-                    $route['handler'],
-                    implode(' ', array_map('class_basename', $filters['before'])),
-                    implode(' ', array_map('class_basename', $filters['after'])),
-                ];
-            }
+            $sampleUri = $uriGenerator->get($route['route']);
+            $filters   = $filterCollector->get($route['method'], $sampleUri);
+            $routeName = ($route['route'] === $route['name']) ? '»' : $route['name'];
+            $tbody[] = [
+                strtoupper($route['method']),
+                $route['route'],
+                $routeName,
+                $route['handler'],
+                implode(' ', array_map('class_basename', $filters['before'])),
+                implode(' ', array_map('class_basename', $filters['after'])),
+            ];
         }

         if ($collection->shouldAutoRoute()) {
    ----------- end diff -----------

Applied rules:
 * RemoveAlwaysTrueIfConditionRector

https://github.com/codeigniter4/CodeIgniter4/actions/runs/5471544778/jobs/9962789821?pr=7650

…tionRector

Without this doc type, Rector will refactor the following:
1) system/Router/AutoRouterImproved.php:294

    ---------- begin diff ----------
@@ @@

             // Update the positions.
             $this->methodPos = $this->paramPos;
-            if ($params === []) {
-                $this->paramPos = null;
-            }
+            $this->paramPos = null;
             if ($this->paramPos !== null) {
                 $this->paramPos++;
             }
    ----------- end diff -----------

Applied rules:
 * RemoveAlwaysTrueIfConditionRector
@kenjis
kenjis force-pushed the skip-rector-AutoRouterImproved branch from 69f1aba to ef1b3b2 Compare July 6, 2023 05:02

@samsonasik samsonasik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you @kenjis

@samsonasik
samsonasik merged commit ceb5976 into codeigniter4:4.4 Jul 6, 2023
@kenjis
kenjis deleted the skip-rector-AutoRouterImproved branch July 6, 2023 05:35
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