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

Bump Symfony versions to ^3.4 and ^4.2 #5733

Merged
merged 5 commits into from
Nov 28, 2019
Merged

Bump Symfony versions to ^3.4 and ^4.2 #5733

merged 5 commits into from
Nov 28, 2019

Conversation

franmomu
Copy link
Member

@franmomu franmomu commented Nov 3, 2019 •
edited
Loading

Subject

As commented here, the current minimal Symfony versions supported has been unmaintained for a while. This PR is about bump those versions to the current maintained versions.

I am targeting this branch, because these changes respect BC.

Changelog

### Removed
- Support for Symfony < 3.4
- Support for Symfony >= 4, < 4.2

I guess this line could be changed as well.

PS: Not sure if changelog is needed I'll add changelog just in case, if not remove it.

EDIT:
To keep track of this change in the other packages, I'll list them here:

@OskarStark
Copy link
Member

PS: Not sure if changelog is needed

@greg0ire WDYT?

@OskarStark
Copy link
Member

Build is 💚 , do we need to change anything on dev-kit ?

@greg0ire
Copy link
Contributor

greg0ire commented Nov 3, 2019

I would use this as a changelog:

### Removed
- Support for Symfony < 3.4
- Support for Symfony >= 4, < 4.2

@OskarStark
Copy link
Member

I updated the PR header

@greg0ire
Copy link
Contributor

greg0ire commented Nov 3, 2019 •
edited
Loading

Can you please go through these queries, see what code you can drop? Apparently there even is code that we should have removed when dropping sf < 2.8

@phansys
Copy link
Member

phansys commented Nov 3, 2019

Can you please go through these queries, see what code you can drop? Apparently there even is code that we should have removed when dropping sf < 2.8

Same for "bump": https://github.com/sonata-project/SonataAdminBundle/search?q=bump&unscoped_q=bump

@phansys phansys added the minor label Nov 3, 2019
@franmomu
Copy link
Member Author

franmomu commented Nov 3, 2019

I would use this as a changelog:

### Removed
- Support for Symfony < 3.4
- Support for Symfony >= 4, < 4.2

mmm Symfony 2.8 is still supported, I guess should be:

Support for Symfony  >= 3.2, < 3.4

@greg0ire
Copy link
Contributor

greg0ire commented Nov 3, 2019

@franmomu we are happy as soon as one LTS is supported, support for 2.8 can be dropped.

@franmomu
Copy link
Member Author

franmomu commented Nov 3, 2019

Perfect! so I'll remove 2.8 as well! 😬

@franmomu franmomu changed the title Bump Symfony versions to ^3.4 and ^4.2 [WIP] Bump Symfony versions to ^3.4 and ^4.2 Nov 3, 2019
@franmomu franmomu force-pushed the bump_symfony_versions branch 5 times, most recently from f77c8ab to 015109f Compare November 3, 2019 20:53
@franmomu
Copy link
Member Author

franmomu commented Nov 3, 2019 •
edited
Loading

Some changes apart from removing code:

  • The throw new changes in one line are automatically made by php-cs-fixer
  • ModelManagerCompilerPassTest and AdminMakerTest had and if with !class_exists('Symfony\Component\Console\CommandLoader\CommandLoaderInterface') so they were never executed because CommandLoaderInterface is an interface (interface_exists should've been used).
  • I added symfony/maker-bundle to composer.json so I guess there should be some changes in dev-kit to remove this job and this line.
  • For this and this looks like the fix was merged in 3.4.30, so we cannot remove it if we don't use that constraint.

@greg0ire
Copy link
Contributor

greg0ire commented Nov 3, 2019

We can bump to 3.4.30, not really an issue if we're bumping to 3.4

@greg0ire
Copy link
Contributor

greg0ire commented Nov 3, 2019

I added symfony/maker-bundle to composer.json

Why? Isn't it supposed to be an optional dependency for us?

@OskarStark
Copy link
Member

No it was optional because not supported with 2.8 IIRC

@greg0ire
Copy link
Contributor

greg0ire commented Nov 3, 2019

Oh ok then.

@franmomu
Copy link
Member Author

franmomu commented Nov 4, 2019 •
edited
Loading

No it was optional because not supported with 2.8 IIRC

That's what I thought and this also just reminded me that we can probably get rid of or deprecate sensio/generator-bundle which doesn't support Symfony 4 and encourage to use MakerBundle (I have to check, but I guess they are do the same thing).

@franmomu franmomu force-pushed the bump_symfony_versions branch 5 times, most recently from 8027ed8 to 00d01b5 Compare November 5, 2019 21:05
@franmomu franmomu force-pushed the bump_symfony_versions branch 2 times, most recently from 0766fab to 7deb61b Compare November 6, 2019 12:19
This was referenced Nov 11, 2019
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@franmomu franmomu force-pushed the bump_symfony_versions branch 4 times, most recently from 3c3147f to 597725b Compare November 12, 2019 23:33
@franmomu franmomu changed the title [WIP] Bump Symfony versions to ^3.4 and ^4.2 Bump Symfony versions to ^3.4 and ^4.2 Nov 13, 2019
@greg0ire greg0ire merged commit fee8ccf into sonata-project:3.x Nov 28, 2019
@greg0ire
Copy link
Contributor

Thanks @franmomu !!!

@franmomu franmomu deleted the bump_symfony_versions branch November 29, 2019 06:42
greg0ire
greg0ire reviewed Dec 1, 2019
View reviewed changes
tests/Maker/AdminMakerTest.php
@@ -109,7 +108,7 @@ public function testExecute(): void
$this->output = new StreamOutput(fopen('php://memory', 'w', false));

$this->io = new ConsoleStyle($this->input, $this->output);
$fileManager = new FileManager(new Filesystem(), '.');
$fileManager = new FileManager(new Filesystem(), new AutoloaderUtil(new ComposerAutoloaderFinder('Sonata\AdminBundle\Tests')), $this->projectDirectory.'/config/'.$this->servicesFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

@franmomu since this particular change, I have a dirty working directory after running the tests:

Untracked files:
  (use "git add <file>..." to include in what will be committed)

	tests/Admin/FooAdmin.php
	tests/Controller/FooAdminController.php

Shouldn't these be generated in /tmp?

Copy link
Member Author

Choose a reason for hiding this comment

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

let me take a look

@franmomu franmomu mentioned this pull request Dec 1, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greg0ire greg0ire greg0ire approved these changes

@core23 core23 core23 approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@franmomu @OskarStark @greg0ire @phansys @core23