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 PHP 8 string functions #8207

Merged
merged 9 commits into from May 16, 2024

Conversation

Oldiesmann
Copy link
Contributor

This changes the code to use str_starts_with, str_contains and str_ends_with instead of strpos and substr for finding strings within other strings. It looks nicer and is less confusing.

@live627
Copy link
Contributor

live627 commented May 10, 2024

Riding the (PHP) storm?

@Oldiesmann
Copy link
Contributor Author

Riding the (PHP) storm?

How did you guess? 😛

@Sesquipedalian
Copy link
Member

If you like, sure. I personally don't mind the old fashioned way for these, but I have no objection.

@jdarwood007
Copy link
Member

I think its a micro improvement:

<?php

$iterations = 100000;

$start = microtime(true);
for($i = 0; $i < $iterations; $i++) {
	$v = substr('Hello World', 0, 5) === 'Hello';
}
$end = microtime(true);

echo 'First:' . $end - $start . "\n";


$start = microtime(true);
for($i = 0; $i < $iterations; $i++) {
	$v = str_starts_with('Hello World', 'Hello');
}
$end = microtime(true);

echo 'Second:' . $end - $start . "\n";

First:0.0059850215911865
Second:0.0034339427947998

Somebody can check the work, but it shows that it does perform slightly better.

@live627
Copy link
Contributor

live627 commented May 10, 2024

Makes sense, because the new functions don't copy strings.

Riding the (PHP) storm?

How did you guess? 😛

PHPStorm automates this. If it were me, I wouldn't do this otherwise because of the tedium, which I do not like.

@Oldiesmann Oldiesmann marked this pull request as ready for review May 11, 2024 03:49
@live627
Copy link
Contributor

live627 commented May 11, 2024

Just discovered this goodie https://cs.symfony.com/doc/rules/alias/modernize_strpos.html

@jdarwood007
Copy link
Member

Neat. Can be added, but requires us to enable risky, but the risk is not a concern:

diff --git a/.php-cs-fixer.dist.php b/.php-cs-fixer.dist.php
index 6fd874e2b..a3b3944a4 100644
--- a/.php-cs-fixer.dist.php
+++ b/.php-cs-fixer.dist.php
@@ -34,6 +34,7 @@ return (new PhpCsFixer\Config())
     ->registerCustomFixers([
         new \SMF\Fixer\Whitespace\closing_tag_fixer(),
     ])
+    ->setRiskyAllowed(true)
        ->setRules([
                '@PSR12' => true,

@@ -156,6 +157,7 @@ return (new PhpCsFixer\Config())
                'explicit_string_variable' => true,
                'simple_to_complex_string_variable' => true,
                'single_quote' => true,
+               'modernize_strpos' => true,

                // Whitespace.
                'array_indentation' => true,

@Sesquipedalian
Copy link
Member

Let's go ahead and enable that option in PHP-CS-Fixer. Then you can just run PHP-CS-Fixer once to get all the remaining instances in one go, @Oldiesmann.

@Oldiesmann
Copy link
Contributor Author

I've already finished everything at this point

@Sesquipedalian
Copy link
Member

Ah. Very good, then!

other/upgrade.php Outdated Show resolved Hide resolved
@Sesquipedalian Sesquipedalian merged commit 4b43091 into SimpleMachines:release-3.0 May 16, 2024
6 checks passed
@Oldiesmann Oldiesmann deleted the php8_string branch May 16, 2024 15:25
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

4 participants