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

use namespaced PHPUnit and keep B.C. #55

Merged
merged 7 commits into from
Oct 25, 2019

Conversation

DQNEO
Copy link
Contributor

@DQNEO DQNEO commented Apr 2, 2018

This is an alternate solution to #52

@@ -2,16 +2,21 @@

namespace Psr\Log\Test;

if (!class_exists('\\PHPUnit_Framework_TestCase', true)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not make sense. You are creating an alias only when the old name does not exist (i.e. in PHPUnit 6+) while you use only the new name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, my bad...
I fixed it. Could you review it again?

@stof
Copy link
Contributor

stof commented Apr 3, 2018

currently, the repo does not have a .gitattribute to exclude the tests from the archive, so every project installing psr/log (most of them today) will have this file with the class_alias in it, which would mess the IDE autocompletion in projects.

So I suggest either keeping this alias out (PHPUnit namespaced classes are available in 4.8.35+, 5.4+ and 6+) or excluding tests from the archive.

@Seldaek what do you think ?

@DQNEO DQNEO closed this Apr 4, 2018
@DQNEO DQNEO deleted the namespaced-phpunit branch April 4, 2018 16:31
@DQNEO DQNEO restored the namespaced-phpunit branch April 4, 2018 18:02
@DQNEO DQNEO reopened this Apr 4, 2018
@Seldaek
Copy link
Collaborator

Seldaek commented Nov 20, 2018

IMO at this point we could merge it without the alias.. This only affects implementors of the PSR which isn't a whole lot of people.

@@ -2,16 +2,21 @@

namespace Psr\Log\Test;

if (!class_exists('\\PHPUnit\\Framework\\TestCase', true)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Class names in PHP shouldn't have the leading slash when written in a string. The implementation of class_exist effectively does an ltrim to remove that slash before continuing. The reason why you might see leading slashes is when the ::class notation is used, because then we are resolving names relative to the current namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! fixed. e50f69c

@michalbundyra
Copy link

@Seldaek Any chance we'll get it sorted finally?
Per your latest comment I guess we want #52 back.

@Seldaek
Copy link
Collaborator

Seldaek commented Aug 24, 2019

Looks good to me now, @stof is the new alias ok or does your comment above still apply regarding IDE confusion?

@Seldaek Seldaek merged commit bf73deb into php-fig:master Oct 25, 2019
@DQNEO DQNEO deleted the namespaced-phpunit branch October 25, 2019 09:18
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.

5 participants