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

improve composer.json and travis tests #57

Merged
merged 8 commits into from Feb 7, 2020
Merged

improve composer.json and travis tests #57

merged 8 commits into from Feb 7, 2020

Conversation

@Tobion
Copy link
Contributor

Tobion commented Feb 5, 2020

  • make dependencies explicit
  • remove compatibility with unmaintained symfony versions
  • define branch alias
  • remove bin config as the binaries from this package are only proxies and do not need to be exposed in other apps
  • not installing symfony/symfony in tests but using symfony/flex#409 (comment)
  • testing with php 7.4
  • fix optional event dispatcher. the event class was instantiated in the consumer/producer even if no dispatcher was injected. since the event class extends symfony event (which class still needs to be fixed for SF 5.0), the event dispatcher component was required to be installed even if not used.
  • fixed test for M6WebAmqpExtension with symfony 4+ as the service are private the tested service gets removed. instead of compiling the container, it should just call load on the extension like https://github.com/FriendsOfSymfony/FOSUserBundle/blob/a9e71b6e7b61e7f39ba77436ba3e9af954450c4b/Tests/DependencyInjection/FOSUserExtensionTest.php#L112-L115
  • add gitattributes based on #52
  • fix missing spaceless tag in twig 3 (replaces #55)
  • it's not compatible with SF 5 yet (DataCollector does not work and base SF event does not exist), see attempt https://github.com/M6Web/AmqpBundle/pull/56/files. but it's not possible to support symfony 3, 4 and 5 at the same time (see symfony/symfony#34230 (comment)). so I suggest to merge this now. and the next step could be to remove support for sf 3 and add support for sf 5.
Tobion added 2 commits Feb 5, 2020
- make dependencies explicit
- remove compatibility with unmaintained symfony versions
- define branch alias
- remove bin config as the binaries from this package are only proxies and do not need to be exposed in other apps
@Tobion Tobion changed the title improve composer.json improve composer.json and travis tests Feb 5, 2020
Copy link
Member

Oliboy50 left a comment

👋 thanks for the PR

LGTM

TODO: fix test for M6WebAmqpExtension with symfony 4+ as the service are private the tested service gets removed

indeed 👍
fact is that we're trying to move from atoum to phpunit (since Symfony is "recommending" the use of phpunit in its ecosystem), so maybe we could also use phpunit in this project too (don't know if it could help)

anyway, the Travis build shows that php7.0 and php7.1 are green, so I think this PR doesn't break anything

@Tobion Tobion force-pushed the Tobion:patch-1 branch from 5f40648 to 0757a8b Feb 6, 2020
@Tobion Tobion force-pushed the Tobion:patch-1 branch from 0757a8b to 98e6f52 Feb 6, 2020
… and removed sf component event class
@Tobion

This comment has been minimized.

Copy link
Contributor Author

Tobion commented Feb 6, 2020

@Oliboy50 this is ready. tests pass. see also my last point in the original description. you could merge this and release it as a new patch release. then the next step would be to support SF 5.

Copy link
Member

Oliboy50 left a comment

thank you again 👏

i'm still not convinced about the .gitattributes file, but I think it won't hurt anyway 👌

I'll let (at least) one of my colleagues approve, merge and release your PR
(the M6Web review policy requires at least 2 approvals before merging)

Copy link
Contributor

b-viguier left a comment

Really nice 👏 👍
Thanks a lot

/Dockerfile export-ignore
/docker export-ignore
/docker-compose.yml export-ignore
/travis export-ignore

This comment has been minimized.

Copy link
@b-viguier

b-viguier Feb 7, 2020

Contributor

By curiosity, in which case is it interesting to prevent the export of these files? 🤔
Is it to provide lighter composer package? If so, what about src/AmqpBundle/Tests/?
Just wondering 😄

This comment has been minimized.

Copy link
@Oliboy50

Oliboy50 Feb 7, 2020

Member

Is it to provide lighter composer package?

indeed :D

#52 (comment)

@lnahiro
lnahiro approved these changes Feb 7, 2020
@lnahiro lnahiro merged commit d3c1d4e into M6Web:master Feb 7, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lnahiro

This comment has been minimized.

Copy link
Contributor

lnahiro commented Feb 7, 2020

release v3.1.2

@Tobion Tobion deleted the Tobion:patch-1 branch Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.