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.

Sign up for GitHub

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

Jump to bottom

Remove sensio generator #5797

Merged
merged 6 commits into from
Dec 20, 2019
Merged

Remove sensio generator #5797

merged 6 commits into from
Dec 20, 2019

Conversation

core23
Copy link
Member

@core23 core23 commented Dec 18, 2019 •
edited
Loading

Subject

I am targeting this branch, because this is a BC break.

Changelog

### Removed
- Remove sensio generator
- Remove `CreateClassCacheCommand`

@core23 core23 added the major label Dec 18, 2019
@core23 core23 force-pushed the remove-sensio branch 3 times, most recently from 1b82040 to 497bb44 Compare December 18, 2019 18:59
@core23
Copy link
Member Author

core23 commented Dec 18, 2019

After removing https://packagist.org/packages/sensio/generator-bundle, we are finally testing symfony 4+. The dev-dependency had locked symfony to version 3 oO

@core23 core23 requested a review from a team December 18, 2019 19:01
@greg0ire
Copy link
Contributor

After removing https://packagist.org/packages/sensio/generator-bundle, we are finally testing symfony 4+. The dev-dependency had locked symfony to version 3 oO

Ouch! Is that why Travis is failing? We should probably add some sanity check for this 😅

greg0ire
greg0ire reviewed Dec 18, 2019
View reviewed changes
Copy link
Contributor

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Looks good, but I think it's far from being the end of it after looking at the tests 😅

@core23
Copy link
Member Author

core23 commented Dec 18, 2019

After removing https://packagist.org/packages/sensio/generator-bundle, we are finally testing symfony 4+. The dev-dependency had locked symfony to version 3 oO

Ouch! Is that why Travis is failing? We should probably add some sanity check for this 😅

No idea. Btw. we have no travis tests for symfony 4 🤔

Looks good, but I think it's far from being the end of it after looking at the tests 😅

I wonder why this code was working, because in production you could use symfony 4

@greg0ire
Copy link
Contributor

No idea. Btw. we have no travis tests for symfony 4 thinking

That's probably something we usually add when a new major gets out, and we should probably challenge that.

@core23
Copy link
Member Author

core23 commented Dec 18, 2019

We need to port some removed test classes here too

@core23 core23 mentioned this pull request Dec 19, 2019
Closed
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

jordisala1991
jordisala1991 reviewed Dec 19, 2019
View reviewed changes
src/Controller/CRUDController.php Outdated
@@ -88,7 +88,7 @@ public function setContainer(ContainerInterface $container = null): void
*
* @deprecated since version 3.27, to be removed in 4.0. Use Sonata\AdminBundle\Controller\CRUDController::renderWithExtraParams() instead.
*/
public function render($view, array $parameters = [], Response $response = null)
public function render(string $view, array $parameters = [], ?Response $response = null): Response
Copy link
Member

Choose a reason for hiding this comment

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

This method should be removed anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right, but I wanted to make the removal more atomic to have a shorter review process

@core23 core23 force-pushed the remove-sensio branch 4 times, most recently from a35f87f to 67db9d1 Compare December 19, 2019 17:10
@core23 core23 force-pushed the remove-sensio branch 2 times, most recently from 8fee709 to 25f7174 Compare December 19, 2019 17:31
@core23 core23 force-pushed the remove-sensio branch 2 times, most recently from e209e30 to f1b92c8 Compare December 19, 2019 18:48
phansys
phansys previously approved these changes Dec 19, 2019
View reviewed changes
@core23
Copy link
Member Author

core23 commented Dec 20, 2019

Can someone please help? Why is the method AbstractWidgetTestCase::getEnvironment final? Can't find the exact version where this method is/was final

@franmomu
Copy link
Member

Can someone please help? Why is the method AbstractWidgetTestCase::getEnvironment final? Can't find the exact version where this method is/was final

Could it be when importing from CoreBundle?

@core23 core23 force-pushed the remove-sensio branch 2 times, most recently from 43087f4 to 9c2b0ce Compare December 20, 2019 15:20
@core23
Copy link
Member Author

core23 commented Dec 20, 2019 •
edited
Loading

Temporary removed the CoreBundle test from Travis, because this will be removed with the next PR #5787

Refs sonata-project/dev-kit#534

@core23 core23 requested a review from a team December 20, 2019 16:02
@core23 core23 merged commit 911607e into sonata-project:master Dec 20, 2019
@core23 core23 deleted the remove-sensio branch December 20, 2019 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordisala1991 jordisala1991 jordisala1991 left review comments

@greg0ire greg0ire greg0ire approved these changes

@phansys phansys phansys left review comments

Assignees
No one assigned
Labels
major
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@core23 @greg0ire @SonataCI @franmomu @jordisala1991 @phansys