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

Update to use new antidot framework package #33

Merged
merged 8 commits into from
Dec 12, 2021

Conversation

kpicaza
Copy link
Member

@kpicaza kpicaza commented Nov 27, 2021

I'm not sure to merge this PR right now, it will break the current starter with 0.x versions. Maybe the best action will be to release the Framework package 1.0.0 as is, and create the Antidot Framework 2 version with the rewrite(merge against 3.x.x instead of 2.x.x see antidot-framework/antidot-framework#41), this way, we can leave the starter 1.0.0 as is, and merge this PR against 3.x.x to create the 2.0.0 version, what do you think about?

@kpicaza kpicaza changed the title Remove cached file and add missed class on config Update to use new antidot framework package Nov 28, 2021
@kpicaza kpicaza self-assigned this Nov 28, 2021
@kpicaza kpicaza added dependencies Pull requests that update a dependency file enhancement New feature or request labels Nov 28, 2021
bin/console Outdated
$_SERVER['APP_RUNTIME'] = AntidotRuntime::class;
$_SERVER['APP_RUNTIME_OPTIONS'] = [
'host' => '0.0.0.0',
'port' => 3000,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should let 3000 port number be used before running the development server.

Or we can have the setting to set these application runtime options.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be configurable from the config 100%

Copy link
Member Author

@kpicaza kpicaza Nov 28, 2021

Choose a reason for hiding this comment

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

Done, now the server is configured as every other component at config/services/dependencies.prod.yaml
image

bin/console Outdated
@@ -4,13 +4,25 @@
declare(strict_types=1);

use Antidot\Cli\Application\Console;
use Antidot\Framework\Runtime\AntidotRuntime;

ini_set('memory_limit', '2048M');
Copy link
Member

Choose a reason for hiding this comment

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

Is this memory setting configurable?

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 put 2048M because when hitting a stress test with concurrency it reaches the default memory limit, it should be configurable by framework config too

Copy link
Member Author

@kpicaza kpicaza Nov 28, 2021

Choose a reason for hiding this comment

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

@kpicaza kpicaza changed the base branch from 2.x.x to 3.x.x November 28, 2021 18:38
"wshafer/psr11-monolog",
"laminas/laminas-httphandlerrunner",
"laminas/laminas-diactoros"
"wshafer/psr11-monolog"
]
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we need to add the Composer script about running development server easily?

And I think it can let Quick Start be a Composer script.

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be something like:

composer dev-server

Copy link
Member

Choose a reason for hiding this comment

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

According to this composer.json script, I think we can define and use composer run dev-server to complete this issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should document this:

image

You can run default host by running:

composer run dev-server

Or run it in custom host by

APP_HOST=127.0.0.1:8080 composer run dev-server

Copy link
Member

@xserrat xserrat left a comment

Choose a reason for hiding this comment

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

🚀 🚀

public/index.php Outdated

require_once $rootDir . '/vendor/autoload_runtime.php';

return static function () use ($rootDir) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return static function () use ($rootDir) {
return static function (): ContainerInterface use ($rootDir) {

use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Server\RequestHandlerInterface;
use Laminas\Diactoros\Response\JsonResponse;

class HomePage implements RequestHandlerInterface
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class HomePage implements RequestHandlerInterface
final class HomePage implements RequestHandlerInterface

@xserrat
Copy link
Member

xserrat commented Dec 4, 2021

I'm not sure to merge this PR right now, it will break the current starter with 0.x versions. Maybe the best action will be to release the Framework package 1.0.0 as is, and create the Antidot Framework 2 version with the rewrite(merge against 3.x.x instead of 2.x.x see antidot-framework/antidot-framework#41), this way, we can leave the starter 1.0.0 as is, and merge this PR against 3.x.x to create the 2.0.0 version, what do you think about?

I think it's a good idea to only maintain the old version for possible vulnerability issues or bugs. Then, the people who want to use the new version I'll use the new tag v3.0. I don't know if it should be Antidot Framework 3 if the branch is 3.x.x. What do you think?

@kpicaza
Copy link
Member Author

kpicaza commented Dec 4, 2021

@xserrat @peter279k I'm not sure about branching convention, I'm following the next logic:

  • 0.x.x tags were made against 1.x.x branch
  • 1.x.x tags against 2.x.x branch
  • so 2.x.x tags against 3.x.x

Maybe is not correct and it should be

  • 0.x.x and 1.x.x tags on 1.x.x branch
  • so 2.x.x to 2.x.x
  • ...

@xserrat
Copy link
Member

xserrat commented Dec 4, 2021

@xserrat @peter279k I'm not sure about branching convention, I'm following the next logic:

  • 0.x.x tags were made against 1.x.x branch

  • 1.x.x tags against 2.x.x branch

  • so 2.x.x tags against 3.x.x

Maybe is not correct and it should be

  • 0.x.x and 1.x.x tags on 1.x.x branch

  • so 2.x.x to 2.x.x

  • ...

Oh, I see. I guess it's also good this convention. It's like all the commits/tags to reach the version <branch_name> are in that branch. For me it's ok, I don't know if it's an standard or not but it makes sense.

@kpicaza
Copy link
Member Author

kpicaza commented Dec 6, 2021

Recently released the dispatcher with the async options. I should do the same with a monolog log handler to ensure that the default pipeline middleware is not blocking the event loop. I have to dig more in-depth into how to write a file using ReactPHP.

I know that writing a file without blocking the event loop is a historical problem on async PHP, but I think that the ReactPHP and Amp people have solved it already;-D.

@kpicaza kpicaza merged commit 9d077f1 into antidot-framework:3.x.x Dec 12, 2021
@kpicaza kpicaza deleted the starter-async branch December 12, 2021 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants