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

TASK: Improve flow cr:status #4853

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

mhsdesign
Copy link
Member

Followup to #4846

  • require a $details string for all non-ok status
  • print "No details available." if verbose and non-ok and no details are available
  • dont wortbreak details in verbose mode but print them with indentation of 2 full line per line
  • print new line after printing details
  • add details to AssetUsageProjection's setupRequired

Followup to neos#4846

- require a $details string for all non-ok status
- print "No details available." if verbose and non-ok and no details are available
- dont wortbreak details in verbose mode but print them with indentation of 2 full line per line
- print new line after printing details
- add details to AssetUsageProjection's setupRequired
@mhsdesign mhsdesign requested review from bwaidelich and dlubitz and removed request for bwaidelich January 23, 2024 11:20
Comment on lines 22 to 23
$emptyString = '';
return new self(CheckpointStorageStatusType::OK, $emptyString);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$emptyString = '';
return new self(CheckpointStorageStatusType::OK, $emptyString);
return new self(CheckpointStorageStatusType::OK, '');

Copy link
Member Author

Choose a reason for hiding this comment

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

see this is a hack

/** @var non-empty-string $emptyString phpstan doesnt support that $details is only a non-empty-string if the type is not ok */

thats why i just fake it for this one case :/

Copy link
Member

Choose a reason for hiding this comment

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

The @param non-empty-string $details in the constructor does not make sense with the empty string. I'd say: remove the param annotation and use Dennys version

Copy link
Member

Choose a reason for hiding this comment

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

Or – does conditional typing work for params, too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes sadly not. I even had a discussion with the phpstan author :D
see phpstan/phpstan#10471

Assertions on properties are not build yet, there is a feature request, maybe ill do it :D
phpstan/phpstan#10463

but until then ill use phpstan ignore next line.
The reason im that strict is to enforce that there is a message for error cases, which should be statically linted.

Copy link
Member

Choose a reason for hiding this comment

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

It does work with some generics magic: https://phpstan.org/r/fbed24c2-e5ba-48ec-a145-5df341986d9f

Copy link
Member

Choose a reason for hiding this comment

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

...but I'm not sure whether that helps in our case

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes i have been there :D

but it doesnt work when passing it to a function:

function fooMe(CheckpointStorageStatus $foo): void
{
    if ($foo->type === CheckpointStorageStatusType::ERROR) {
        // how to teach phpstan this relation?
	\PHPStan\Testing\assertType('non-empty-string', $foo->details);
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

thats why i went with this compromise ... i hope its okay.

@@ -7,23 +7,34 @@
*/
final readonly class CheckpointStorageStatus
{
/**
* @param non-empty-string $details
Copy link
Member

Choose a reason for hiding this comment

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

That should be removed IMO

Copy link
Member Author

Choose a reason for hiding this comment

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

the reason its there to enforce that there is a message for error cases, which should be statically enforced.
Otherwise we might lazy and forget it :D

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise we cant write this

Parameter #1 $details of static method Neos\ContentRepository\Core\Projection\ProjectionStatus::error() expects non-empty-string, string given.

$checkpointStorageStatus = $this->checkpointStorage->status();
if ($checkpointStorageStatus->type === CheckpointStorageStatusType::ERROR) {
    return ProjectionStatus::error($checkpointStorageStatus->details);
}

@mhsdesign mhsdesign force-pushed the task/4846followupImproveCrStatus branch from a6faefa to 6e4d015 Compare February 2, 2024 14:47
@mhsdesign mhsdesign force-pushed the task/4846followupImproveCrStatus branch from 6e4d015 to e75b771 Compare February 2, 2024 14:49
@mhsdesign
Copy link
Member Author

CI fails because #4870 is not merged

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

Fine for me, thanks for taking care!

@mhsdesign mhsdesign merged commit 95b601b into neos:9.0 Feb 2, 2024
5 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants