Skip to content

Conversation

@svycka
Copy link
Contributor

@svycka svycka commented Oct 5, 2017

fix for #1671 should not break alias but also should fix IDE problems at least phpStorm works. lets see if tests pass

fix for squizlabs#1671 should not break alias but also should fix IDE problems at least phpStorm works. lets see if tests pass
@svycka
Copy link
Contributor Author

svycka commented Oct 5, 2017

okay, PHP_CodeSniffer is not happy about this workaround but maybe we could ignore this or implement in PHP_CodeSniffer friendly way? Anyway is this good way to fix this?

@svycka
Copy link
Contributor Author

svycka commented Oct 5, 2017

Also if we don't want to fix this then maybe we can inverse this and create aliases, not for new phpunit versions but for old then only people with legacy versions will have problems lets hope there are fewer people

@jrfnl
Copy link
Contributor

jrfnl commented Oct 5, 2017

Also if we don't want to fix this then maybe we can inverse this

It's been set up so that it will be easy to drop support for older PHPUnit versions when the time comes. Inversing the aliases breaks that.

@svycka
Copy link
Contributor Author

svycka commented Oct 5, 2017

@jrfnl don't know how this would be more difficult to drop support older versions but maybe don't know everything. Then what about this workaround of a workaround? :)

@jrfnl
Copy link
Contributor

jrfnl commented Oct 5, 2017

Then what about this workaround of a workaround?

For what it's worth: IMHO this is a shortcoming/issue with the IDE and not something which should be solved in PHPCS.
If something should be done in PHPCS at all about this, I'd be in favour of adding the tests back into the .gitattributes file so they don't ship by default.

@svycka
Copy link
Contributor Author

svycka commented Oct 5, 2017

I understand that this is not a phpcs problem but this issue is really annoying also easy to fix and don't expect this to be fixed in all IDE's soon. But I am +1 to adding tests to .gitattributes.

@gsherwood gsherwood merged commit 16ace84 into squizlabs:master Oct 8, 2017
gsherwood added a commit that referenced this pull request Oct 8, 2017
@gsherwood
Copy link
Member

I've merged this in. Is anyone able to test and see if it is working.

Before anyone decides to comment, I'm aware this is a hacky fix. I really don't have the time to put effort into something like this. As long as it works, I'm happy for now.

@svycka
Copy link
Contributor Author

svycka commented Oct 10, 2017

dev-master no longer shows duplicate implementations so works for me. But That's my PR so guess this doesn't count...

@gsherwood
Copy link
Member

That's my PR so guess this doesn't count...

If you're the only one who confirms it, it counts a lot! Thanks for testing.

@gsherwood
Copy link
Member

Confirmed a second time on main report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants