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

[Parallel] Fix system errors expected an instance of SystemError but got string #1610

Merged
merged 4 commits into from Jan 4, 2022
Merged

[Parallel] Fix system errors expected an instance of SystemError but got string #1610

merged 4 commits into from Jan 4, 2022

Conversation

zingimmick
Copy link
Contributor

@TomasVotruba
Copy link
Member

The fix should happen in the other direction. Where the error message is string, should be object.

@zingimmick
Copy link
Contributor Author

zingimmick commented Jan 1, 2022

The fix should happen in the other direction. Where the error message is string, should be object.

@TomasVotruba I tried to convert it to an object, but the required argument $relativeFilePath is missing.

The location of the code where the error message is string:

$systemErrors[] = 'System error: ' . $jsonError;

$systemErrors[] = 'Child process error: ' . $stdErr;

$systemErrors[] = sprintf(
'Reached system errors count limit of %d, exiting...',
self::SYSTEM_ERROR_COUNT_LIMIT
);

@TomasVotruba
Copy link
Member

but the required argument $relativeFilePath is missing.

It could be either optional or with with AbstractSystemError. How would that work?

The reason is we should work with strict objects, moreover when we juggle around json data in paralle processes. Only that way we can control what data types are on I/O. When we start to send strings, it could be anything, from error message, to file content, and we'll be unable to resolve what we work with.

Jsonable value objects makes sure we know the types all the time.

@zingimmick
Copy link
Contributor Author

The reason is we should work with strict objects, moreover when we juggle around json data in paralle processes. Only that way we can control what data types are on I/O. When we start to send strings, it could be anything, from error message, to file content, and we'll be unable to resolve what we work with.

I see. I'll try the optional first and see what happens.

@zingimmick
Copy link
Contributor Author

This is what an error message looks like with relativeFilePath optional.

 [ERROR] Could not process "" file, due to:                                                                             
         "Reached system errors count limit of 20, exiting...".    

@TomasVotruba
Copy link
Member

How should we rephrase it?

@@ -166,7 +166,7 @@ function (array $json) use (
// decode arrays to objects
foreach ($json[Bridge::SYSTEM_ERRORS] as $jsonError) {
if (is_string($jsonError)) {
$systemErrors[] = 'System error: ' . $jsonError;
$systemErrors[] = new SystemError('System error: ' . $jsonError);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you type this array, so phpstan would error when strings are pushed onto the array?

Copy link
Contributor Author

@zingimmick zingimmick Jan 3, 2022

Choose a reason for hiding this comment

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

Add a type to this array, but phpstan does not error when a string is pushed into the array. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not supported yet. phpstan/phpstan#3269, phpstan/phpstan#3703

@zingimmick
Copy link
Contributor Author

How should we rephrase it?

How about

 [ERROR] Could not process some files, due to:                                                                             
         "Reached system errors count limit of 20, exiting...".    

@TomasVotruba
Copy link
Member

@zingimmick Sounds good 👍

@TomasVotruba
Copy link
Member

Thank you 👍

I'm merging this now, so we can fix important bug that blocks us at the moment. Feel free to send PR with better wording.

@TomasVotruba TomasVotruba merged commit 373afba into rectorphp:main Jan 4, 2022
@zingimmick zingimmick deleted the fix-error-is-string branch January 4, 2022 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants