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

[DRAFT]: Remove ExtraFrameworkBundle #599

Closed
wants to merge 1 commit into from
Closed

[DRAFT]: Remove ExtraFrameworkBundle #599

wants to merge 1 commit into from

Conversation

toxicity1985
Copy link
Contributor

@toxicity1985 toxicity1985 commented Dec 13, 2023

Hello,

I have working on the way to remove the dependencies to ExtraFrameworkBundle.
I create an AnnotationListener to get the annotation from the controller and get a few stuff from ExtraFrameworkBundle.
I have some issue with the test but i have no idea why.
I know that's related to the AbstractRuleListener. The matchRule is not working as expected.
If you have some idea what am i wrong ?
It's a draft version.

image

use Symfony\Component\HttpFoundation\Response;

class RuleMatcherTest extends TestCase
{
public function testRequestMatcherCalled()
{
$requestMatcher = new RequestMatcher(null, null, null, null, ['_controller' => '^AcmeBundle:Default:index$']);
$requestMatcher = new AttributesRequestMatcher(['_controller' => '^AcmeBundle:Default:index$']);
Copy link
Contributor

Choose a reason for hiding this comment

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

ah right, we need to fix those for symfony 7.

the new class is only available since Symfony 6.2 can you please do class_exists(AttributesRequestMatcher::class) ? new AttributesRequestMatcher(...) : new RequestMatcher(...); to keep legacy tests working?

and i would prefer this in a separate pull request as it is not related to extra framework bundle. it is fixing a deprecation in symfony 6 and avoid the error in symfony 7.

@dbu
Copy link
Contributor

dbu commented Dec 13, 2023

oh snap. i thought we could get rid of the explicit references to FrameworkExtraBundle but keep using it for the annotations if it is available. but thats because i recently upgraded to attributes in a library that used the doctrine annotations implementation.

i think porting the annotation things from extra bundle into this bundle is adding too much unrelated legacy code. i think after all we should do a new major version that simply drops annotations support. i created the 3.x branch for a new major version.

sorry for the work i made you do - i hope it was at least interesting to see how it works 🙈

can you target 3.x with the fixes for the specific request matchers? we don't need the class_exists after all, we should simply make the 3.x version only allow symfony 6.4 and 7. and attributes should be simply deleting the annotation things and they will only have the #[\Attribute... information.

@toxicity1985
Copy link
Contributor Author

No worries about the double work. It was ok to go deeper inside this bundle.
So il will remove all the stuff related to annotation and make a new pull request for that.
I will also make a new one for the request matcher issue.

@dbu
Copy link
Contributor

dbu commented Dec 13, 2023

thank you very much @toxicity1985 !

@toxicity1985
Copy link
Contributor Author

i can close this one.

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