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

Test sf4 #4742

Merged
merged 8 commits into from
Nov 7, 2017
Merged

Test sf4 #4742

merged 8 commits into from
Nov 7, 2017

Conversation

greg0ire
Copy link
Contributor

@greg0ire greg0ire commented Nov 4, 2017

Do not merge, this is a proof PR. the dev-kit PR has been merged, so this should be mergeable too.

@greg0ire
Copy link
Contributor Author

greg0ire commented Nov 4, 2017

This can be merged now, but I need to fix a test on SonataCore for the sf3.4 build to be green.

soullivaneuh
soullivaneuh previously approved these changes Nov 4, 2017
Copy link
Member

@soullivaneuh soullivaneuh 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 would like at least another review for this one.

core23
core23 previously approved these changes Nov 4, 2017
@greg0ire
Copy link
Contributor Author

greg0ire commented Nov 4, 2017

This can be merged now, but I need to fix a test on SonataCore for the sf3.4 build to be green.

See sonata-project/SonataCoreBundle#457

@soullivaneuh ideally, merge that one before, then release, and let me bump the version constraint in this PR. If you think this is going to be slow, then please merge it now instead.

@greg0ire
Copy link
Contributor Author

greg0ire commented Nov 4, 2017

Green: https://travis-ci.org/sonata-project/SonataAdminBundle/jobs/297337761 :)

soullivaneuh
soullivaneuh previously approved these changes Nov 4, 2017
@greg0ire
Copy link
Contributor Author

greg0ire commented Nov 4, 2017

This allows testing sf4. Currently it breaks because I need to do yet another correction on the abstract widget tests case, because I got no deprecation warning regarding initRuntime, which is removed in sf4. I'd rather do that and fix other sf4-related deprecations, but for that I need sonata-project/SonataCoreBundle#456. This can be merged now, at least it will fix the deprecations build, and make sf4 testable by Travis.

@SonataCI
Copy link
Collaborator

SonataCI commented Nov 5, 2017

Could you please rebase your PR and fix merge conflicts?

@@ -479,14 +490,20 @@ public function testProcessAbstractAdminServiceInServiceDefinition()
->setArguments(['', 'Sonata\AdminBundle\Tests\DependencyInjection\Post', ''])
->setAbstract(true);

$adminDefinition = new DefinitionDecorator('sonata_abstract_post_admin');
$adminDefinition = class_exists(ChildDefinition::class) ?
Copy link
Contributor

@jlamur jlamur Nov 6, 2017

Choose a reason for hiding this comment

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

This is a BC trick for SF > 3.3 SF < 3.3, please add a NEXT_MAJOR comment.

->setClass('Sonata\AdminBundle\Tests\DependencyInjection\MockAbstractServiceAdmin')
->setArguments([0 => 'extra_argument_1'])
->addTag('sonata.admin', ['group' => 'sonata_post_one_group', 'manager_type' => 'orm']);

$adminTwoDefinition = new DefinitionDecorator('sonata_abstract_post_admin');
$adminTwoDefinition = class_exists(ChildDefinition::class) ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

$definitionOrReference = $articleRouteBuilderMethodCall[1][0];
if ($definitionOrReference instanceof Definition) {
$this->assertSame(
'Sonata\DoctrinePHPCRAdminBundle\Route\PathInfoBuilderSlashes',
Copy link
Member

Choose a reason for hiding this comment

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

::class notation.

This fixes a deprecation triggered while running tests.
The compiler pass that injects default dependencies needs it.
Definitions no longer know their service id in Symfony 4
This will allow running some tests on them.
It comes with a fix that will make some deprecations go away.
@OskarStark
Copy link
Member

No travis test with SF 4.0 beta?

@greg0ire
Copy link
Contributor Author

greg0ire commented Nov 7, 2017

@OskarStark it's called dev-master ;)

@soullivaneuh soullivaneuh merged commit e8a840c into sonata-project:3.x Nov 7, 2017
@greg0ire greg0ire deleted the test_sf4 branch November 7, 2017 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants