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

Remove deprecated assertContains #32977

Merged

Conversation

MarioBlazek
Copy link
Contributor

@MarioBlazek MarioBlazek commented Aug 6, 2019

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

This PR replaces assertContains() with assertStringContainsString().

@nicolas-grekas
Copy link
Member

Thanks, we just merged #32971 which does the same on 3.4
Can you please rebase to target 4.3? I'm sure there are more there.

@MarioBlazek MarioBlazek changed the base branch from 4.4 to 4.3 August 6, 2019 08:22
@MarioBlazek MarioBlazek force-pushed the deprec-phpunit-assertContains branch from 7b09a9e to 622c638 Compare August 6, 2019 08:22
@MarioBlazek
Copy link
Contributor Author

@nicolas-grekas PR rebased against 4.3

@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Aug 6, 2019
@nicolas-grekas
Copy link
Member

All the cases that fail with "mb_strpos() expects parameter 1 to be string, array given" should use assertContainsEquals instead

@nicolas-grekas
Copy link
Member

Wait, assertContains is not deprecated. Some uses of it are. This should be more selective about those.

$this->assertContains('BaseBundle:controller:custom.format.engine', $templates);
$this->assertContains('::this.is.a.template.format.engine', $templates);
$this->assertContains('::resource.format.engine', $templates);
$this->assertStringContainsString('BaseBundle::base.format.engine', $templates);
Copy link
Member

Choose a reason for hiding this comment

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

templates is an array, you should revert and keep assertContains.

The deprecation is about using assertContains when the second argument is a string (ie. assertContains('foo', 'foobarbaz') )
Same comment for many changes bellow.

An easy way to spot theme, is to run test and revert reverting the failing tests ;-)

@jderusse jderusse mentioned this pull request Aug 6, 2019
1 task
@MarioBlazek
Copy link
Contributor Author

Thank you guys, I will update PR accordingly asap.

@MarioBlazek
Copy link
Contributor Author

@nicolas-grekas @jderusse tests now pass, but build for some strange reason is hanging too long. Did you experienced something similar before?

@nicolas-grekas
Copy link
Member

@MarioBlazek tests should be back to OK with #33005

@MarioBlazek
Copy link
Contributor Author

Tests are now OK, thanks @nicolas-grekas

@nicolas-grekas
Copy link
Member

Thank you @MarioBlazek.

@nicolas-grekas nicolas-grekas merged commit 43acda6 into symfony:4.3 Aug 7, 2019
nicolas-grekas added a commit that referenced this pull request Aug 7, 2019
This PR was squashed before being merged into the 4.3 branch (closes #32977).

Discussion
----------

Remove deprecated assertContains

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32844
| License       | MIT
| Doc PR        | symfony

This PR replaces `assertContains()` with `assertStringContainsString()`.

Commits
-------

43acda6 Remove deprecated assertContains
nicolas-grekas added a commit that referenced this pull request Aug 8, 2019
This PR was merged into the 4.3 branch.

Discussion
----------

Fix deprecations on 4.3

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32844
| License       | MIT
| Doc PR        | NA

Fix deprecations in branch 4.3
note: remaining deprecation `assertStringContainsString` will be fixed in #32977

* [ ] fix tests in branch 3.4 in #32981

Commits
-------

8fd16a6 Fix deprecation on 4.3
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.

None yet

4 participants