gbprod / elastica-bundle

Really simple bundle to use Elastica within Symfony applications
Do What The F*ck You Want To Public License
4 stars 0 forks source link

Fixed web toolbar widget integration #23

Closed FlyingDR closed 6 years ago

FlyingDR commented 6 years ago

This pull request fixes issue #22 with web toolbar integration and adds tests for proper integration of services of the bundle into Symfony.

codecov-io commented 6 years ago

Codecov Report

Merging #23 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             master    #23   +/-   ##
=======================================
  Coverage       100%   100%           
  Complexity       45     45           
=======================================
  Files             4      4           
  Lines           139    139           
=======================================
  Hits            139    139

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 08473ec...77882fe. Read the comment docs.

FlyingDR commented 6 years ago

@gbprod Into commit ba542e5 I've removed Symfony 3.0 from tests matrix and replaced it with 3.4 LTS. Since this is partially out of scope of this pull request - here are reasons for this change:

  1. Symfony 3.0 is already unsupported for about a year.
  2. There was no 3.4 LTS version in tests
  3. There was, unfortunately, not simple path to make tests compatible with Symfony 3.0, they was broken on PHP 7.x.

There is at least 2 reasons for tests to be broken:

  1. Symfony 3.0.x requires PHPUnit 5.7- "plain" class names while PHPUnit 6.x with namespaced classes is being installed for PHP 7.x. Can be solved by explicitly defining PHPUnit versions, but is it required for already unsupported version?
  2. There was a change made somewhere around Symfony 3.1.4 that, when paired with Twig version constraints results into failure into Twig extensions loading. Workaround would require changes that are not related to the bundle itself just to let tests run on already unsupported version of Symfony.

Hope these reasons are good enough to approve this change.

gbprod commented 6 years ago

Thanks for you're contribution, I'll take some times to review before merge !

gbprod commented 6 years ago

Seems that you've not updated the CHANGELOG.md file. Can you add the changes description ?

gbprod commented 6 years ago

Thanks @FlyingDR for this contribution, I really appreciate :)