Rector deletes contents of file when it encounters an error

[devbanana]

Bug Report

Subject	Details
Rector version	0.11.48

This one is hard to debug because it seems intermittent, but I'll describe how I got there.

Basically I was using RenameClassRector to move some classes from one namespace to another. I couldn't use RenameNamespaceRector because only a few classes were moving to the new location. Anyway the important thing is that the class kept the same name, just in a new namespace.

Anyway for a few files which used these moved classes (not for all of them for some reason, just a few), it got into a state where the use statement was added for the new namespace, but the old was not removed.

If I then ran Rector again, it threw an error and deleted the whole contents of the file.

So for example if I had a class Foo\Old\Bar and wanted to move it to Foo\New\Bar, some other file, like a unit test, might include this:

use Foo\Old\Bar; // should be deleted but wasn't
use Foo\New\Bar; // added by Rector

Then when Rector was run on this, it would encounter an error and delete the whole contents of the file (only a blank file was left).

I ran this on a whole directory and didn't catch that it had deleted some of my files until several commits later.

The main thing to be fixed here I think is that a file's contents shouldn't be deleted when an error is encountered.

I'm sorry I can't provide more useful information.

[TomasVotruba]

Hi, thanks for repoting. It's hard to guess where exactly is the problem and what we need to fix.
For such a serious bug we'll need a reproducible repository. Could you share minimal ~10 lines example in github repository?

[lulco]

I fall into same issue when I forgot —autoload-file option. No matter which rule I used.

[TomasVotruba]

That cannot happen. Could you share an repository with this bug?

[devbanana]

It's difficult since this was for work and I don't want to share such code publicly. I'll see if I can create an example that occurs reliably though.

[TomasVotruba]

Thank you 👍

[lulco]

My case you can reproduce pretty simple:

cd /var/www/
composer create-project nette/web-project rector-example
cd /path/to/rector-src (git cloned repository rector-src)
bin/rector process /var/www/rector-example/app/Presenters/ErrorPresenter.php --dry-run

Content disappears:

 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
1 file with changes
===================

1) ../../../../rector-example/app/Presenters/ErrorPresenter.php

    ---------- begin diff ----------
@@ @@
-<?php
-
-declare(strict_types=1);
-
-namespace App\Presenters;
-
-use Nette;
-use Nette\Application\Responses;
-use Nette\Http;
-use Tracy\ILogger;
-
-
-final class ErrorPresenter implements Nette\Application\IPresenter
-{
-	use Nette\SmartObject;
-
-	/** @var ILogger */
-	private $logger;
-
-
-	public function __construct(ILogger $logger)
-	{
-		$this->logger = $logger;
-	}
-
-
-	public function run(Nette\Application\Request $request): Nette\Application\Response
-	{
-		$exception = $request->getParameter('exception');
-
-		if ($exception instanceof Nette\Application\BadRequestException) {
-			[$module, , $sep] = Nette\Application\Helpers::splitName($request->getPresenterName());
-			return new Responses\ForwardResponse($request->setPresenterName($module . $sep . 'Error4xx'));
-		}
-
-		$this->logger->log($exception, ILogger::EXCEPTION);
-		return new Responses\CallbackResponse(function (Http\IRequest $httpRequest, Http\IResponse $httpResponse): void {
-			if (preg_match('#^text/html(?:;|$)#', (string) $httpResponse->getHeader('Content-Type'))) {
-				require __DIR__ . '/templates/Error/500.phtml';
-			}
-		});
-	}
-}
    ----------- end diff -----------

                                                                                                                        
 [ERROR] Could not process "../../../../rector-example/app/Presenters/ErrorPresenter.php" file, due to:                 
         "PHPStan\BetterReflection\Reflection\ReflectionClass "Nette\Application\IPresenter" could not be found in the  
         located source". On line: 24                                                                                   
                                                                                                                       

And then the same with --autoload-file option

bin/rector process /var/www/rector-example/app/Presenters/ErrorPresenter.php --dry-run --autoload-file /var/www/rector-example/vendor/autoload.php

It works!

 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
1 file with changes
===================

1) ../../../../rector-example/app/Presenters/ErrorPresenter.php

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

 namespace App\Presenters;

+use Nette\Application\IPresenter;
+use Nette\SmartObject;
+use Nette\Application\Request;
+use Nette\Application\Response;
+use Nette\Application\BadRequestException;
+use Nette\Application\Helpers;
+use Nette\Application\Responses\ForwardResponse;
+use Nette\Application\Responses\CallbackResponse;
+use Nette\Http\IRequest;
+use Nette\Http\IResponse;
+use Nette\Utils\Strings;
 use Nette;
 use Nette\Application\Responses;
 use Nette\Http;
@@ @@
 use Tracy\ILogger;


-final class ErrorPresenter implements Nette\Application\IPresenter
+final class ErrorPresenter implements IPresenter
 {
-	use Nette\SmartObject;
+	use SmartObject;

-	/** @var ILogger */
-	private $logger;

-
-	public function __construct(ILogger $logger)
+	public function __construct(private ILogger $logger)
 	{
-		$this->logger = $logger;
 	}


-	public function run(Nette\Application\Request $request): Nette\Application\Response
+	public function run(Request $request): Response
 	{
 		$exception = $request->getParameter('exception');

-		if ($exception instanceof Nette\Application\BadRequestException) {
-			[$module, , $sep] = Nette\Application\Helpers::splitName($request->getPresenterName());
-			return new Responses\ForwardResponse($request->setPresenterName($module . $sep . 'Error4xx'));
+		if ($exception instanceof BadRequestException) {
+			[$module, , $sep] = Helpers::splitName($request->getPresenterName());
+			return new ForwardResponse($request->setPresenterName($module . $sep . 'Error4xx'));
 		}

 		$this->logger->log($exception, ILogger::EXCEPTION);
-		return new Responses\CallbackResponse(function (Http\IRequest $httpRequest, Http\IResponse $httpResponse): void {
-			if (preg_match('#^text/html(?:;|$)#', (string) $httpResponse->getHeader('Content-Type'))) {
+		return new CallbackResponse(function (IRequest $httpRequest, IResponse $httpResponse): void {
+			if (Strings::match((string) $httpResponse->getHeader('Content-Type'), '#^text/html(?:;|$)#')) {
 				require __DIR__ . '/templates/Error/500.phtml';
 			}
 		});
    ----------- end diff -----------

Applied rules:
 * ArgumentAdderRector
 * RemoveUnusedPromotedPropertyRector
 * PregMatchFunctionToNetteUtilsStringsRector
 * AddSeeTestAnnotationRector
 * ClassPropertyAssignToConstructorPromotionRector (https://wiki.php.net/rfc/constructor_promotion https://github.com/php/php-src/pull/5291)
 * UnionTypesRector


                                                                                                                        
 [OK] 1 file would have changed (dry-run) by Rector                                                                     
                                                                                                                 

[lulco]

Probably this error cause it:

[ERROR] Could not process "../../../../rector-example/app/Presenters/ErrorPresenter.php" file, due to:                 
         "PHPStan\BetterReflection\Reflection\ReflectionClass "Nette\Application\IPresenter" could not be found in the  
         located source". On line: 24

But if there is such error, Rector should not delete content, just throw error.

What was your error @devbanana?

[devbanana]

Yes, my error looked very similar to yours @lulco . When Rector throws an error like that it deletes the content of the file.

[samsonasik]

I created PR rectorphp/rector-src#716 , please try that and verify if that fixes the issue.

[lulco]

Works for me, thx

Maybe @frankdejonge has same issue? https://twitter.com/frankdejonge/status/1419577807823384576