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

Add TypeInferenceTest for current_time() #76

Merged
merged 12 commits into from
Apr 19, 2023

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Apr 16, 2023

@IanDelMar @szepeviktor @johnbillion

maybe this is useful to continue with szepeviktor/phpstan-wordpress#182 / szepeviktor/phpstan-wordpress#180?

still missing: phpcs and all the crazy CS rules that Viktor has in the extension repo 😊

tests/data/current_time.php Outdated Show resolved Hide resolved
@szepeviktor
Copy link
Member

szepeviktor commented Apr 17, 2023

So all functions from functionMap.php should have this kind of tests?

@herndlm
Copy link
Contributor Author

herndlm commented Apr 17, 2023

I think we can start moving tests over for extensions that will be removed. Also newly added things should have tests I suppose, yes. Depends :)

@szepeviktor
Copy link
Member

szepeviktor commented Apr 17, 2023

image

Every 20th WordPress installation is version 4.9.
I feel we need to raise major version (to 2) of phpstan-wordpress to make these changes.

@herndlm
Copy link
Contributor Author

herndlm commented Apr 17, 2023

what for exactly? sorry, I didn't understand it yet. this change is not affecting consumers in any way, right? it's just adding tests closer to the type definition because @IanDelMar correctly pointed out that it's super weird to have them in the other repo still

and the repo here doesn't even know about phpstan-wordpress yet, dependency-wise all should be clean IMO

UPDATE: ah you mean that if we remove the extensions from phpstan-wordpress older WP versions will loose those types because this repo here is only having them for newer WP versions. something like that, right? That's definitely a valid argument against removing them in phpstan-wordpress I suppose.. but adding tests here still makes sense, right? :)

@johnbillion
Copy link
Contributor

I think you could just publish a major version increment for phpstan-wordpress (2.0.0) and then if someone needs those types for WordPress 4.9 they'll just have to use the existing v1 branch.

@IanDelMar
Copy link
Contributor

Can I commit to this PR to add tests for other functions?

@herndlm
Copy link
Contributor Author

herndlm commented Apr 18, 2023

Can I commit to this PR to add tests for other functions?

of course, go ahead! I just wanted to check here if Viktor accepts this and move only one test over for now

@IanDelMar
Copy link
Contributor

Tbh I was not asking for permission but whether it works. I get an authentication failure.

@herndlm
Copy link
Contributor Author

herndlm commented Apr 18, 2023

Tbh I was not asking for permission but whether it works. I get an authentication failure.

I added you as collaborator to my fork. but I never did that before. if it doesn't work, feel free to just create a new branch / PR, I'll close this one then

@IanDelMar
Copy link
Contributor

Works now!

@szepeviktor
Copy link
Member

szepeviktor commented Apr 18, 2023

Please use a vendor+package style namespace. PhpStubs\WordPress\Core\Tests

.travis.yml Outdated Show resolved Hide resolved
phpunit.xml.dist Outdated Show resolved Hide resolved
Comment on lines 21 to 22
/** @var string $string */
$string = null;
Copy link
Member

@szepeviktor szepeviktor Apr 18, 2023

Choose a reason for hiding this comment

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

Could (string)$_GET['unknown_string'] work here?
To avoid introducing a new variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as we exclude the test data from being analyzed with PHPStan it works. PHPStan would complain about an maybe undefined index.

Copy link
Member

Choose a reason for hiding this comment

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

$_GET is used in other test data files.

@szepeviktor
Copy link
Member

May I press Merge now?

@IanDelMar
Copy link
Contributor

May I press Merge now?

Tests for get_post() are still missing but we can add them in another PR.

@szepeviktor szepeviktor merged commit ceabc90 into php-stubs:master Apr 19, 2023
@szepeviktor
Copy link
Member

szepeviktor commented Apr 19, 2023

Now WordPress got a bit better 🙃

Thank you Martin and Ian!

@herndlm herndlm deleted the add-type-inference-test branch April 19, 2023 10:46
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.

4 participants