Skip to content

Feat/core - #105

Open
MicaelMnl wants to merge 10 commits into
Linkvalue:masterfrom
MicaelMnl:feat/core
Open

Feat/core#105
MicaelMnl wants to merge 10 commits into
Linkvalue:masterfrom
MicaelMnl:feat/core

Conversation

@MicaelMnl

@MicaelMnlMicaelMnl commented Feb 5, 2020•
edited
Loading

Copy link
Copy Markdown

Breaking changes:

  • Remove support vagrant
  • Add docker environment for local
  • Add support Heroku
  • Remove support PHP 5.6 and 7.0
  • Update PHP to 7.1
  • Update Nodejs to 12
  • Update Symfony to 3.4 with dependencies

Features:

  • Replace parameters.yml to .env
  • Add Procfile file

Oliboy50
Oliboy50 reviewed Feb 5, 2020
View reviewed changes

@Oliboy50Oliboy50 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👏 very good job 👏
thank you ❤️

Comment threadCONTRIBUTING.md Outdated
Comment threadCONTRIBUTING.md Outdated
Comment threadCONTRIBUTING.md Outdated
Comment threadCONTRIBUTING.md Outdated
Comment threadCONTRIBUTING.md Outdated
Comment threaddocker/node/Dockerfile Outdated
Comment threaddocker-compose.yml Outdated
Comment thread.env Outdated
Comment threadCONTRIBUTING.md Outdated
Comment threadCONTRIBUTING.md Outdated
@MicaelMnl
MicaelMnlforce-pushed the feat/core branch 6 times, most recently from 147d2ef to 30945a3CompareFebruary 7, 2020 14:04
@aiKrice

aiKrice commented Feb 7, 2020

Copy link
Copy Markdown
Contributor

Thanks for keeping alive this project ❤️

@MicaelMnl
MicaelMnlforce-pushed the feat/core branch 4 times, most recently from 6657246 to 2552cbdCompareFebruary 11, 2020 11:07
Oliboy50
Oliboy50 reviewed Feb 11, 2020
View reviewed changes
Comment thread.env.dist Outdated
Comment threadapp/config/config.yml Outdated
Comment threadapp/config/config.yml Outdated
Comment threadapp/AppKernel.php Outdated
Comment threadapp/AppKernel.php Outdated
Comment thread.env.dist
Comment threadbin/console Outdated
Comment threadcomposer.json Outdated
Comment threadcomposer.json
"scripts": {
"symfony-scripts": [
"Incenteev\\ParameterHandler\\ScriptHandler::buildParameters",
"cp .env.dist .env",

@Oliboy50Oliboy50Feb 11, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could we remove "incenteev/composer-parameter-handler" from "require" then?

@MicaelMnlMicaelMnlFeb 18, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes

@Oliboy50Oliboy50Feb 18, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so... could you do it in this PR, please? 😅

Comment threaddoc/configuration/nginx.md Outdated
@MicaelMnl
MicaelMnlforce-pushed the feat/core branch 9 times, most recently from 9df4f9f to ac2e688CompareFebruary 18, 2020 14:14
Oliboy50
Oliboy50 reviewed Feb 18, 2020
View reviewed changes

@Oliboy50Oliboy50 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

almost there 🙆‍♂

Comment threadapp/AppKernel.php
$loader->load(function (ContainerBuilder $container) {
// Check if webpack dev server is up before using it
@file_get_contents($container->getParameter('webpack_dev_server_base_url'));
@file_get_contents($container->resolveEnvPlaceholders($container->getParameter('webpack_dev_server_base_url'), true));

@Oliboy50Oliboy50Feb 18, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
@file_get_contents($container->resolveEnvPlaceholders($container->getParameter('webpack_dev_server_base_url'), true));
@file_get_contents($container->getParameter('webpack_dev_server_base_url'));

no need to resolve because we already convert the env variable to a parameter:

webpack_dev_server_base_url: '%env(WEBPACK_DEV_SERVER_BASE_URL)%'

and this is a URL, so we don't need to resolve anything 🤔

Comment threadapp/AppKernel.php
'base_url' => sprintf(
'%s/%s',
rtrim($container->getParameter('webpack_dev_server_base_url'), '/'),
rtrim($container->resolveEnvPlaceholders($container->getParameter('webpack_dev_server_base_url'), true), '/'),

@Oliboy50Oliboy50Feb 18, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
rtrim($container->resolveEnvPlaceholders($container->getParameter('webpack_dev_server_base_url'), true), '/'),
rtrim($container->getParameter('webpack_dev_server_base_url'), '/'),

Comment threadcomposer.json
"scripts": {
"symfony-scripts": [
"Incenteev\\ParameterHandler\\ScriptHandler::buildParameters",
"cp .env.dist .env",

@Oliboy50Oliboy50Feb 18, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so... could you do it in this PR, please? 😅

Comment threadweb/app_dev.php
$dotenv = new Dotenv();
$dotenv->load(__DIR__.'/../.env');

if($_ENV['APP_ENV'] !== 'prod') {

@Oliboy50Oliboy50Feb 18, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why don't we just delete this file (app_dev.php), instead of doing weird things like that?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@Oliboy50Oliboy50Oliboy50 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MicaelMnl@aiKrice@Oliboy50