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

Removed the try/catch that used to handle the AnnotationException(s) #176

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

antiftw
Copy link
Contributor

@antiftw antiftw commented Jun 14, 2024

This commit is code-cleanup after the Symfony7 bump. I took another look into the code because I wanted to figure out where the Exception was thrown regarding the unknown Attributes.

It indeed confirmed that that specific Exception was not thrown anymore. However, another exception seems to be able to be thrown, so I'm now at least properly catching that. Maybe we want to add a new test for this? Although I'm not really certain what the Exception indicates. Looking into that as we speak.

P.S. Not really sure why it includes all the other commits in this PR too, even though the changed code is only from the last one. Don't hesitate to tell me if I did something wrong/forgot to do something. I updated my master with the last commit from this one, but maybe because I did the other commit first?

@antiftw antiftw closed this Jun 14, 2024
@antiftw antiftw reopened this Jun 14, 2024
Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

because I did the other commit first?

Hm, the diff looks good, probably so. Probably cherry-pick and squash commits may help, not sure.

Maybe we want to add a new test for this?

This would be definitely cool 👍

@antiftw
Copy link
Contributor Author

antiftw commented Jun 14, 2024

Hm, the diff looks good, probably so. Probably cherry-pick and squash commits may help, not sure.

You can squash when merging the PR right?

I've also looked into the NoSuchMetadataException, and inside the function we are calling it (MetadataFactoryInterface->getMetadataFor()) it is thrown twice:

Once if the argument is neither an object or a string:

if (!\is_object($value) && !\is_string($value)) {
   throw new NoSuchMetadataException(sprintf(
      'Cannot create metadata for non-objects. Got: "%s".',
      get_debug_type($value)
   ));
}

and then it also checks if its neither a class or an interface:

if (!class_exists($class) && !interface_exists($class, false)) {
   throw new NoSuchMetadataException(sprintf(
      'The class or interface "%s" does not exist.', 
      $class
   ));
}

So that seems like we would have to pass an integer, bool, float or array to that function but that does not really make sense to me in our context, because if you would put an attribute above a simple variable you will just get a syntax error.

Also, since we load our files using namespaces, I'm not even sure how we would get our variables there, since that assumes that the file contains a class, which would imply that there will be no exception thrown.

I'd like to hear what you think about this?

@antiftw antiftw changed the title Removed the try catch that used to catch the AnnotationErrors Removed the try catch that used to catch the AnnotationException(s) Jun 14, 2024
@antiftw antiftw changed the title Removed the try catch that used to catch the AnnotationException(s) Removed the try/catch that used to catch the AnnotationException(s) Jun 14, 2024
@antiftw antiftw changed the title Removed the try/catch that used to catch the AnnotationException(s) Removed the try/catch that used to handle the AnnotationException(s) Jun 14, 2024
@bocharsky-bw
Copy link
Member

You can squash when merging the PR right?

Yep, GitHub should squash it.

that does not really make sense to me in our context, because if you would put an attribute above a simple variable you will just get a syntax error

Hm, yeah, probably that can be simplified unless we're missing something here. But I can't think of anything.

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

Successfully merging this pull request may close these issues.

2 participants