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

401 php label #403

Closed
wants to merge 3 commits into from
Closed

Conversation

thedavidmeister
Copy link
Contributor

Exactly the same as #402 but against a dedicated branch. Fixes #401

@henriquemoody
Copy link
Member

Could you please create PHPLabel.md on "docs" directory?

@henriquemoody henriquemoody added this to the 1.0 milestone Oct 6, 2015
public function testInvalidPHPLabelShouldThrowPhoneException($input)
{
$this->assertFalse($this->phpLabelValidator->__invoke($input));
$this->assertFalse($this->phpLabelValidator->assert($input));

Choose a reason for hiding this comment

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

Just to point out, all assert methods on PHPUnit are static. This might stop working at some point in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's not going to happen, see sebastianbergmann/phpunit#1914 (comment)

Choose a reason for hiding this comment

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

Doesn't depend on him, but rather on PHP itself becoming stricter over time and hacks like this being removed; which admittedly won't happen just yet. Regardless of what Sebastian Bergmann says, $this-> to access a static method is wrong and might stop working in the future.

PHPUnit's hardly an example of good php code.

Copy link
Member

Choose a reason for hiding this comment

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

When/if it happens a single line of shell script can change the entire test suite and, since it's not BC break, because it's on the test suite, it can be changed in any version.

I think you're right (on many points), but until it happens I think we should stick with what is a common and widely accepted standard, don't you think?

Choose a reason for hiding this comment

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

You're right indeed 👍 . Just trying to raise awareness as lots of people haven't realised never having looked at PHPUnit assertions code.

@henriquemoody henriquemoody modified the milestones: Backlog, 1.0 Oct 24, 2015
@batusa batusa mentioned this pull request Mar 4, 2016
@henriquemoody
Copy link
Member

Closed by f40eb63

Thanks for contributing! 🐼

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.

3 participants