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

Symfony 4 Compatibility #476

Open
wants to merge 8 commits into
base: 3.0
from

Conversation

Projects
None yet
4 participants
@macroprog

macroprog commented Nov 18, 2017

No description provided.

@cristianoc72

This comment has been minimized.

Show comment
Hide comment
@cristianoc72

cristianoc72 Jan 10, 2018

Member

Please, could you re-run travis? After merging propelorm/Propel2#1434 test suite should be green.
Thank you!

Member

cristianoc72 commented Jan 10, 2018

Please, could you re-run travis? After merging propelorm/Propel2#1434 test suite should be green.
Thank you!

<argument>%kernel.root_dir%</argument>
<argument>%propel.configuration%</argument>
<argument type="service" id="faker.generator" on-invalid="null" />
<argument type="service" id="faker.generator" on-invalid="null" public="true"/>

This comment has been minimized.

@gharlan

gharlan Jan 10, 2018

Contributor

I guess the public="true" is wrong here (arguments don't have a public attribute).

@gharlan

gharlan Jan 10, 2018

Contributor

I guess the public="true" is wrong here (arguments don't have a public attribute).

@macroprog

This comment has been minimized.

Show comment
Hide comment
@macroprog

macroprog Jan 27, 2018

My last commit is green, I left you see if it's usefull to merge it to the master...

macroprog commented Jan 27, 2018

My last commit is green, I left you see if it's usefull to merge it to the master...

@cristianoc72

This comment has been minimized.

Show comment
Hide comment
@cristianoc72

cristianoc72 Jan 28, 2018

Member

Thank you to include my branch 4.0 into your PR!
Anyway, I'm asking you (and the PropelBundle team) to consider the following points:

  1. I refactored the bundle to be Symfony 4.0 only, so it's better to create a new 4.0 branch in Propel repository, instead of merging it into 3.0, because it beaks compatibility with Symfony 3.
  2. My 4.0 branch is not finished yet: I have to test the commands behavior with the new directory structure and some configuration properties should be moved from configuration file to .ENV file. I plan to do this in a couple of days.
  3. Since Symfony 4 directory structure is different from previous versions, there are some changes in PropelBundle 4.0 branch that should be documented. I plan to write a new recipe for Propel cookbook at the end of the next week.
  4. Last but not the least, I'm working on a Symfony flex recipe for PropelBundle, so that we could install PropelBundle and Propel simply with composer require propel.

Please, have a little patience again 😄

Member

cristianoc72 commented Jan 28, 2018

Thank you to include my branch 4.0 into your PR!
Anyway, I'm asking you (and the PropelBundle team) to consider the following points:

  1. I refactored the bundle to be Symfony 4.0 only, so it's better to create a new 4.0 branch in Propel repository, instead of merging it into 3.0, because it beaks compatibility with Symfony 3.
  2. My 4.0 branch is not finished yet: I have to test the commands behavior with the new directory structure and some configuration properties should be moved from configuration file to .ENV file. I plan to do this in a couple of days.
  3. Since Symfony 4 directory structure is different from previous versions, there are some changes in PropelBundle 4.0 branch that should be documented. I plan to write a new recipe for Propel cookbook at the end of the next week.
  4. Last but not the least, I'm working on a Symfony flex recipe for PropelBundle, so that we could install PropelBundle and Propel simply with composer require propel.

Please, have a little patience again 😄

@kingchills

This comment has been minimized.

Show comment
Hide comment
@kingchills

kingchills Feb 2, 2018

Contributor

I too have been working on a 4.0 branch for this. Please feel free to use any of my work on this. I have implemented registering of the commands, and have tested most of them. My unit tests all come back green. One of my concerns of your current work is setting all of the services to be public, which isn't considered best practices within symfony. Also your changes in Command/AbstractCommand.php are unnecessary if files are placed in the proper place (schemas will be found and moved to the cache if placed in src/propel/*schema.xml).

Contributor

kingchills commented Feb 2, 2018

I too have been working on a 4.0 branch for this. Please feel free to use any of my work on this. I have implemented registering of the commands, and have tested most of them. My unit tests all come back green. One of my concerns of your current work is setting all of the services to be public, which isn't considered best practices within symfony. Also your changes in Command/AbstractCommand.php are unnecessary if files are placed in the proper place (schemas will be found and moved to the cache if placed in src/propel/*schema.xml).

"symfony/security-acl": "^2.8|^3.0|^4.0"
},
"suggest": {
"symfony/security-acl": "For using acl instead of voters"

This comment has been minimized.

@kingchills

kingchills Feb 2, 2018

Contributor

Does this need to be in the "suggest" if it is in the require?

@kingchills

kingchills Feb 2, 2018

Contributor

Does this need to be in the "suggest" if it is in the require?

"phpunit/phpunit": "^6.0",
"sensio/framework-extra-bundle": "^4.0",
"fzaninotto/faker": "^1.5",
"symfony/security-acl": "^3.0",

This comment has been minimized.

@kingchills

kingchills Feb 2, 2018

Contributor

Does not need to be in "require-dev" when in require

@kingchills

kingchills Feb 2, 2018

Contributor

Does not need to be in "require-dev" when in require

@cristianoc72

This comment has been minimized.

Show comment
Hide comment
@cristianoc72

cristianoc72 Feb 5, 2018

Member

@kingchills your work is almost perfect, and it's compatible with Symfony 3.4, too! Why don't you submit a PR? Imho, your 4.0 branch is already mergeable, as is.

Member

cristianoc72 commented Feb 5, 2018

@kingchills your work is almost perfect, and it's compatible with Symfony 3.4, too! Why don't you submit a PR? Imho, your 4.0 branch is already mergeable, as is.

@kingchills

This comment has been minimized.

Show comment
Hide comment
@kingchills

kingchills Feb 5, 2018

Contributor

@cristianoc72 Consider it done.

Contributor

kingchills commented Feb 5, 2018

@cristianoc72 Consider it done.

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