Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jul 6, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jul 3, 2023

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
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
Copy link
Member Author

kenjis commented Jul 5, 2023

@samsonasik Created rectorphp/rector#8039

@samsonasik
Copy link
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
Copy link
Member Author

kenjis commented Jul 6, 2023

@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
Copy link
Member Author

kenjis commented Jul 6, 2023

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
Copy link
Member

@samsonasik samsonasik left a comment

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants