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

PHP 8 named arguments support #548

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

fadrian06
Copy link
Contributor

@fadrian06 fadrian06 commented Feb 23, 2024

PHP named arguments helps in some situations for cleaned code, it locks library maintainers to not change the argument names so easily without breaking changes, but php 8 must have support too...

public function test_static_route(): void {
  Flight::request()->url = '/test';

  $route = Flight::route(
    pass_route: true,
    alias: 'testRoute',
    callback: function () {
      echo 'test';
    },
    pattern: '/test'
  );

  self::assertInstanceOf(Route::class, $route);
  self::expectOutputString('test');
  Flight::start();
}

@fadrian06 fadrian06 changed the title Php8 named arguments support PHP 8 named arguments support Feb 23, 2024
Copy link
Collaborator

@n0nag0n n0nag0n left a comment

Choose a reason for hiding this comment

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

So I see what you're doing here and I think it's great for the future version of Flight, but it breaks the 7.4 compatibility even for just unit testing. Because most of my machines are configured for 7.4 (because of the LTS support), I couldn't actually pass unit tests anymore if this change was merged in. I'd have to always manually skip that class, which is kind of defeating the point of having that unit test.

The other thing with the Flight::path() change. I know you had mentioned in the past to get rid of the comments and actually build static methods for each of them, and I guess we can do that, but all the methods would just be return self::$app->whatever() or like you put with static::__callStatic('whatever') and seem like a lot of duplicated code, to just add some comments.

@fadrian06
Copy link
Contributor Author

Ok to start the tests for php 8 are excluded from the main suite, I run the tests of that specific class by hand

@fadrian06
Copy link
Contributor Author

Calling the app directly instead of __callStatic does make sense because it would help the static analyzer

@fadrian06
Copy link
Contributor Author

20240223_015803.jpg

20240223_015800.jpg

@fadrian06
Copy link
Contributor Author

I have several ideas to solve it but I don't know how difficult it is to implement it and keep everything working as is.

  • Extract shared functionalities into traits (it violates SOLID everywhere, although the Flight class does it but in an elegant way delegating everything to the Dispatcher, the Loader and the Engine)

  • It could be by extracting everything into a single trait that is automatically generated with an automated script, so with each docblock that is added, the static method will be created

@fadrian06 fadrian06 closed this Apr 2, 2024
@fadrian06 fadrian06 reopened this Apr 3, 2024
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.

2 participants