Skip to content

Add Behat test for PR 2183 #2188

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
Merged
merged 1 commit into from
Apr 6, 2019
Merged

Add Behat test for PR 2183 #2188

merged 1 commit into from
Apr 6, 2019

Conversation

ilicmsreten
Copy link
Contributor

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

This test is related with approved PR and reported bug #799.

@ilicmsreten ilicmsreten mentioned this pull request Sep 4, 2018
Closed
@soyuka
Copy link
Member

soyuka commented Sep 4, 2018

Nice, many thanks! looks like tests are failing on deps=LOW though, is this feature limited to a given version of doctirne? If that's the case, we should maybe bump that dependency...

dunglas
dunglas requested changes Sep 5, 2018
View reviewed changes
composer.json Outdated
@@ -36,7 +36,7 @@
"behatch/contexts": "3.1.0",
"doctrine/annotations": "^1.2",
"doctrine/doctrine-bundle": "^1.8",
"doctrine/orm": "^2.5.2",
"doctrine/orm": "^2.6.2",
Copy link
Member

Choose a reason for hiding this comment

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

Isn't 2.6.0 enough? It would be better to set as minimal version the first Doctrine version supporting this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dunglas yes. I edit composer.json

composer.json Outdated
@@ -54,7 +54,7 @@
"symfony/console": "^3.4 || ^4.0",
"symfony/debug": "^2.8 || ^3.0 || ^4.0",
"symfony/dependency-injection": "^3.4 || ^4.0",
"symfony/doctrine-bridge": "^2.8.12 || ^3.0 || ^4.0",
"symfony/doctrine-bridge": "^4.0",
Copy link
Member

Choose a reason for hiding this comment

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

If it is supported by Symfony 4.0, it should also be supported by Symfony 3.4. We should at least support 3.4 because it's the latest LTS version.

@ilicmsreten ilicmsreten force-pushed the add-behat-test-for-PR-2183 branch from 2f16aa4 to e0f1a7f Compare September 5, 2018 08:12
@soyuka
Copy link
Member

soyuka commented Sep 5, 2018

Mhh maybe we should only push this (and #2183) onto master because of versions bump @dunglas wdyt?

@dunglas
Copy link
Member

dunglas commented Sep 6, 2018

I agree with @soyuka, it may cause big issues for existing projects. @ilicmsreten do you think we can use some kind of feature detection instead of bumping the version? If I understand correctly the patch, bumping the version if only necessary for tests, not for the actual code? If true, we may just skip these tests when old versions are used. WDYT?

@ilicmsreten
Copy link
Contributor Author

@dunglas for old versions PR will not fix bug, but for that PR is not need to bump versions.
So test will pass after bump versions, but we can skip test when old versions is used.

@dunglas
Copy link
Member

dunglas commented Sep 11, 2018 •
edited
Loading

but we can skip test when old versions is used.

👍 Let's do this!

@soyuka soyuka mentioned this pull request Sep 17, 2018
Merged
@antograssiot
Copy link
Contributor

Given #2205 I guess this can be closed ?

@ilicmsreten
Copy link
Contributor Author

@antograssiot yes cause #2205, but this test need to pass after fixing bug api-platform/api-platform#799.
This is just test for api-platform/api-platform#799 (after all there no needs to edit composer.json).

@soyuka
Copy link
Member

soyuka commented Sep 18, 2018 •
edited
Loading

Given #2205 I guess this can be closed ?

We still want the hydration feature, we just need to move it elsewhere (ie not in the eager loading extension because there's no pagination yet...)

/note for EU-FOSSA Hackathon just cherry-pick the test

@antograssiot
Copy link
Contributor

I'm working on finishing this one

@antograssiot antograssiot force-pushed the add-behat-test-for-PR-2183 branch from e0f1a7f to bf2ce57 Compare April 6, 2019 09:53
@soyuka soyuka changed the base branch from 2.3 to 2.4 April 6, 2019 09:54
@antograssiot antograssiot force-pushed the add-behat-test-for-PR-2183 branch 3 times, most recently from 6eec21a to 44af515 Compare April 6, 2019 10:26
@antograssiot antograssiot force-pushed the add-behat-test-for-PR-2183 branch from 44af515 to 8029b86 Compare April 6, 2019 12:15
@soyuka soyuka merged commit 9eafd71 into api-platform:2.4 Apr 6, 2019
@soyuka
Copy link
Member

soyuka commented Apr 6, 2019

thanks @antograssiot @ilicmsreten

@ilicmsreten ilicmsreten deleted the add-behat-test-for-PR-2183 branch April 6, 2019 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dunglas dunglas dunglas requested changes

@soyuka soyuka soyuka approved these changes

Assignees
No one assigned
Labels
⭐ EU-FOSSA Hackathon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ilicmsreten @soyuka @dunglas @antograssiot