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

[NodeBundle] Deprecate unused TemplateEngine constructor argument #2073

Merged

Conversation

acrobat
Copy link
Member

@acrobat acrobat commented Aug 9, 2018

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Fixed tickets

This was a left over of the clean up in PR #544. I've deprecated the unused $templating argument so it can be removed in the next major.

I wanted to add a test to check the deprecation behavior. But this is not possible because we run our tests with codeception. (see explanation below)

<?php

namespace Kunstmaan\NodeBundle\Tests\EventListener;

use Doctrine\ORM\EntityManagerInterface;
use Kunstmaan\NodeBundle\EventListener\RenderContextListener;
use Symfony\Component\Templating\EngineInterface;

class RenderContextListenerTest extends \PHPUnit\Framework\TestCase
{
    /**
     * @group legacy
     * @expectedDeprecation Passing a templating engine as the first argument of "Kunstmaan\NodeBundle\EventListener\RenderContextListener::__construct" is deprecated since KunstmaanNodeBundle 5.1 and will be removed in KunstmaanNodeBundle 6.0. Remove the template engine service argument.
     */
    public function test__construct()
    {
        $templateEngine = $this->createMock(EngineInterface::class);
        $em = $this->createMock(EntityManagerInterface::class);

        new RenderContextListener($templateEngine, $em);
    }
}

The test checks the deprecation behavior by using the @expectedDeprecation annotation provided by symfony/phpunit-bridge. This annotation allows to test classes if they throw the correct deprecation warning. But in the current codeception setup it's not possible to use this listener as codeception doesn't use the phpunit.xml (where you should register the phpunit-bridge listener) or you have no option to manually set the listener in the codeception config.

Is it even necessary to run our testsuite with codeception? As we currently only have phpunit tests and integration tests can easily be done with behat (as it is already the case on the standard-edition). I would suggest to switch back to phpunit as we can cover all our cases with phpunit and have the extra test helpers from the symfony/phpunit-bridge

@ProfessorKuma ProfessorKuma added this to the 5.1.0 milestone Aug 9, 2018
Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR needs some changes

  • This PR seems to need a milestone of a minor release.

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR passed all our requirements.

Thank you for contributing!

@Devolicious Devolicious merged commit 7628b73 into Kunstmaan:master Aug 21, 2018
@acrobat acrobat deleted the deprecate-unused-constructor-arg branch August 21, 2018 13:00
Devolicious added a commit that referenced this pull request Aug 21, 2018
* master:
  [NodeBundle] Deprecate unused TemplateEngine constructor argument (#2073)
  update changelog
  New Crowdin translations (#1948)
  [KunstmaanNodeSearchBundle] use hosts in useVersion6 check (#2075)
  [MediaBundle] add return to load function (#2076)
  [MenuBundle] Fix menu item sorting issue with multiple menus (#2072)
acrobat added a commit to acrobat/KunstmaanBundlesCMS that referenced this pull request Aug 24, 2018
acrobat added a commit to acrobat/KunstmaanBundlesCMS that referenced this pull request Aug 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants