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

Attempt to fix incorrect doctrine table attribute values on php 8.1 #2699

Merged
merged 2 commits into from Jul 21, 2022

Conversation

acrobat
Copy link
Contributor

@acrobat acrobat commented Jul 21, 2022

Reverted some changes of #1850 as they resulted in incorrect doctrine Table attribute values on php 8.1 (indexes and uniqueConstraints can't be initialized inline) + some extra needed fixes

Fixes rectorphp/rector#7163 and partial fix of rectorphp/rector-doctrine#116 (point 1 and 2)

I've updated the failing test of rector-doctrine in rectorphp/rector-doctrine#118

acrobat added a commit to acrobat/rector-doctrine that referenced this pull request Jul 21, 2022
TomasVotruba pushed a commit to rectorphp/rector-doctrine that referenced this pull request Jul 21, 2022
@TomasVotruba
Copy link
Member

Thank you for the PR. The rector-doctrine now passed well 👍

One Rector run is still failing though. Could you fix it?

@acrobat
Copy link
Contributor Author

acrobat commented Jul 21, 2022

Well I tried to debug and fix it but I don't understand it 😅 The bin/rector process packages command fails when I removed the __constructor body of \Rector\PhpAttribute\NodeFactory\PhpAttributeGroupFactory 🤔 Do you have an idea @TomasVotruba?

@TomasVotruba
Copy link
Member

Have you tried to dump the line it refers?

"Rector\Core\PhpParser\Node\Value\ValueResolver::resolveClassConstFetch()":264"        

Then I'd try to narrow down what file or code is causing it.

bin/rector p packages/Exact/File.php

Then dump the node before last crash and you'll see the broken code snippet.

@acrobat
Copy link
Contributor Author

acrobat commented Jul 21, 2022

Debug info

That's indeed what I did but I don't understand why it fails.

bin/rector process packages/PhpAttribute/NodeFactory/PhpAttributeGroupFactory.php

The error is throw in this part of the code

private function resolveClassConstFetch(ClassConstFetch $classConstFetch)
{
$class = $this->nodeNameResolver->getName($classConstFetch->class);
$constant = $this->nodeNameResolver->getName($classConstFetch->name);
if ($class === null) {
throw new ShouldNotHappenException();
}
if ($constant === null) {
throw new ShouldNotHappenException();
}
if ($class === ObjectReference::SELF) {
$classLike = $this->betterNodeFinder->findParentType($classConstFetch, ClassLike::class);
if (! $classLike instanceof ClassLike) {
throw new ShouldNotHappenException();
}
$class = (string) $this->nodeNameResolver->getName($classLike);
}

I've dumped the resolved $class and. $classConstant values on line 264 and these are

string(4) "self"
string(21) "UNWRAPPED_ANNOTATIONS"

And therefor the $classLike is null and the exception is thrown. I don't understand as there is no constant UNWRAPPED_ANNOTATIONS, only a property $unwrappedAnnotations. What can you dump more to maybe get more specific info on where it goes wrong in \Rector\PhpAttribute\NodeFactory\PhpAttributeGroupFactory?

Or does rector try to convert the private property to a constant as it is not used anymore to assign a value? But it fails somehow

EDIT: Changing it a constant instead of a property does indeed seem to fix it!

@TomasVotruba
Copy link
Member

Great job 👍

Could you push the feature and fix commits separately? I'm curious what is the fix for next time this happens :)

@acrobat
Copy link
Contributor Author

acrobat commented Jul 21, 2022

Done!

@TomasVotruba
Copy link
Member

TomasVotruba commented Jul 21, 2022

Thanks 👍

it's interesting, that should not be a problem for Rector.
Both version are valid and should be resolved witout fail.

I'll check this one later and merge this along with it, so I don't forget the bug in another rule 👍 Thank you

@TomasVotruba
Copy link
Member

The solution is on the way 😀 👍
See #2781

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants