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

Simplify and update phpstan configuration #99

Merged
merged 2 commits into from
Jul 6, 2020
Merged

Conversation

szepeviktor
Copy link
Contributor

@szepeviktor szepeviktor commented Jul 6, 2020

Makes PHPStan return "OK"


This change is Reviewable

@coveralls
Copy link

coveralls commented Jul 6, 2020

Coverage Status

Coverage remained the same at 80.786% when pulling fe695e0 on szepeviktor:patch-1 into 5445d44 on voku:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 80.786% when pulling ffd47a7 on szepeviktor:patch-1 into 5445d44 on voku:master.

@szepeviktor
Copy link
Contributor Author

On PHP 7.3 with PHP_COMPOSER_SETUP=lowest PHP_EXTENSION=false PHPStan complains.

 ------ -------------------------------------------------------------------- 

  Line   voku/helper/UTF8.php                                                

 ------ -------------------------------------------------------------------- 

  8313   Function mb_str_split not found.                                    

 ------ -------------------------------------------------------------------- 

How could it be?

@szepeviktor
Copy link
Contributor Author

Could it be this change?

  • use "mb_str_split" with PHP >= 7.4 + mbstring support (performance++)

@voku
Copy link
Owner

voku commented Jul 6, 2020

@szepeviktor Thanks for the clean-up and I think phpstan did not understand the if (Bootup::is_php('7.4')) { check.

@voku voku merged commit 0828e1f into voku:master Jul 6, 2020
@szepeviktor szepeviktor deleted the patch-1 branch July 7, 2020 03:51
@szepeviktor
Copy link
Contributor Author

I think if (Bootup::is_php('7.4')) { is not executed on PHP 7.3.
That is why mb_str_split() is not available.

@szepeviktor
Copy link
Contributor Author

...I see!

If we teach is_php() to PHPStan it may cease to complain.

@szepeviktor
Copy link
Contributor Author

@voku Please see the conversion on this matter: phpstan/phpstan#3576 (comment)

What do you think about function_exists() instead of Bootup::is_php() ?

@voku
Copy link
Owner

voku commented Jul 7, 2020

function_exists should work and we can also cache the result, so that we do not check this every time.

@szepeviktor
Copy link
Contributor Author

szepeviktor commented Jul 7, 2020

we can also cache the result, so that we do not check this every time

1M function_exists calls take 0.064774036407471 seconds on my slow server.

<?php

$start = microtime(true);
for ($run = 0; $run < 1e6; $run += 1) {
    $exists = function_exists('mb_str_split');
}
echo microtime(true) - $start, "\n";

So 1 call takes 64 nanosec :)

voku added a commit that referenced this pull request Jul 7, 2020
… with ```\function_exists('mb_str_split')```

-> #99 (comment)
@voku
Copy link
Owner

voku commented Jul 8, 2020

@szepeviktor I added the function_exists check but phpstan still complains about Function mb_str_split not found. (https://travis-ci.org/github/voku/portable-utf8/jobs/705962575) :-/ I think I did something wrong because I can't see it on my local system?

@szepeviktor
Copy link
Contributor Author

szepeviktor commented Jul 8, 2020

I think I did something wrong because I can't see it on my local system?

No! That is fine.
The above linked PHPStan issue is a feature request.

Currently the only solution is to define the function for PHPStan only.

parameters:
    scanFiles:
        - tests/phpstan/functions.php

A stub is enough.

@szepeviktor
Copy link
Contributor Author

I can't see it on my local system?

Maybe your PHP version is not 7.3 ...

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.

None yet

3 participants