Patch add str_starts_with and str_ends_with #5300

wkhudgins92 · 2020-03-25T23:20:34Z

This is the patch for the following RFC: https://wiki.php.net/rfc/add_str_starts_with_and_ends_with_functions

Contains the str_starts_with and str_ends_with functions as well as tests.

Overall this is a narrowed-down revival of https://wiki.php.net/rfc/add_str_begin_and_end_functions and #2049 .

update fork

glensc · 2020-03-26T08:09:08Z

@wkhudgins92 the wiki states that php8 has been released, can you link that text or correct it. as to my knowledge php8 is not yet released (to be released in December 2020):

PHP8 has now been released and several individuals have requested the str_starts_with and str_ends_with functionality again. This is a simple but highly desired functionality for PHP.

ext/standard/string.c

ext/standard/basic_functions.stub.php

guilliamxavier · 2020-03-26T09:35:21Z

Contains the str_starts_with and str_ends_with functions as well as tests.

Did you forget to commit the tests? 😅 (also please include tests for empty strings and NUL byte, like https://github.com/php/php-src/pull/5179/files#diff-efcf36fef88a923719524d7a5c568689)

wkhudgins92 · 2020-03-26T15:57:36Z

Contains the str_starts_with and str_ends_with functions as well as tests.

Did you forget to commit the tests? 😅 (also please include tests for empty strings and NUL byte, like https://github.com/php/php-src/pull/5179/files#diff-efcf36fef88a923719524d7a5c568689)

Ooops I forgot to commit the tests...I was really rushing to get this out last night, I knew if I didn't get it raise up last night it might take me another week to do anything on it! These are the tests from the old RFC I raised, they don't include empty strings or NULL byte but I can add those.

wkhudgins92 · 2020-03-26T15:58:13Z

@wkhudgins92 the wiki states that php8 has been released, can you link that text or correct it. as to my knowledge php8 is not yet released (to be released in December 2020):

PHP8 has now been released and several individuals have requested the str_starts_with and str_ends_with functionality again. This is a simple but highly desired functionality for PHP.

Fixed that this morning, thanks.

guilliamxavier

Thanks for the tests ;) You can indeed complete them (and address nikic's review) when you get some time again

ext/standard/string.c

ext/standard/tests/strings/str_ends_with.phpt

ext/standard/string.c

ext/standard/tests/strings/str_ends_with.phpt

wkhudgins92 · 2020-03-31T01:25:27Z

@guilliamxavier I added to the test cases as you requested and fixed the typos you found. To be honest, I'm not 100% sure I did the NULL BYTE tests properly, but that was my best guess at the time. I tested hour Python and Java handle startswith and endswith functionality, and at least with empty strings it is the same. I'll get to @nikic idea of using memcmp for str_ends_with soon.

guilliamxavier · 2020-03-31T08:05:56Z

Thank you =) The idea of empty strings tests is basically that both ("a", "") and ("", "") give true while ("", "a") gives false (for both str_{starts,ends}_with like for str_contains), so the ones you added seem OK. Concerning NUL byte tests, the goal is to ensure that the functions are "binary safe" in that they handle \0 just like any other char (and don't crash nor stop at it), e.g. that str_starts_with gives true for ("a\x00bc", "a\x00b") (no crash) but false for all of ("a\x00b", "a\x00d"), ("a\x00b", "z\x00b"), ("a", "a\x00") and ("a", "\x00a") (no stop), and similarly for str_ends_with (with reversed strings), maybe as a complement of those you added.

wkhudgins92 · 2020-04-01T00:42:16Z

@guilliamxavier I added some more to the null byte test cases. All my test seem to be passing its just one of the Travis builds fails with this error:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received
The build has been terminated

Its failed like that at least two times in a row, so not sure what is going on.

I also changed str_ends_with to utilize a call to memcmp as suggested by @nikic , I believe I got that right. Unless other things come up on the PR, I'm going to incorporate some feedback I've received into the RFC page next.

ext/standard/tests/strings/str_starts_with.phpt

ext/standard/tests/strings/str_ends_with.phpt

ext/standard/tests/strings/str_starts_with.phpt

guilliamxavier · 2020-04-01T08:17:49Z

ext/standard/string.c

+    }
+
+    k = ZSTR_LEN(haystack) - ZSTR_LEN(needle);
+    RETURN_BOOL(memcmp(&(ZSTR_VAL(haystack))[k], ZSTR_VAL(needle), ZSTR_LEN(needle)) == 0);


&(ZSTR_VAL(haystack))[k] seems "non idiomatic" in this file, I would rather see
ZSTR_VAL(haystack) + ZSTR_LEN(haystack) - ZSTR_LEN(needle) and get rid of k entirely
(but let's see what nikic thinks)

I'm alright with changing it, I don't do a lot of C so I had to play around some to figure that one out. More than happy to use a more concise or idiomatic way.

ext/standard/tests/strings/str_ends_with.phpt

ext/standard/string.c

guilliamxavier

Thanks for updating the PR. I still think that both test files should be kept "symmetrical", so (and this should be my last review of tests) I have added applicable suggestions:

ext/standard/tests/strings/str_ends_with.phpt

ext/standard/tests/strings/str_starts_with.phpt

wkhudgins92 · 2020-04-09T20:52:07Z

Thanks for the suggestions, @guilliamxavier I incorporated them into the tests!

ext/standard/tests/strings/str_starts_with.phpt

TysonAndre · 2020-05-05T13:36:02Z

@wkhudgins92 - I'd strongly suggest that you squash the 22 commits in https://github.com/php/php-src/pull/5300/commits so that it can get merged

Other RFCs are generally merged as a single commit when possible to do so, e.g. 1668ad7
It makes sense to have each commit work as a change on its own - some of these changes only make sense in combination with other changes (e.g. "changed spaces to tabs")
Attempting to merge this branch into master locally results in merge conflicts - Some commits change ext/standard/php_string.h and then revert the change.

nikic

This looks basically fine, I'll apply final fixups while merging.

nikic · 2020-05-05T13:58:59Z

ext/standard/string.c

+PHP_FUNCTION(str_ends_with) 
+{
+	zend_string *haystack, *needle;
+	int k;


This needs to use size_t, otherwise it may truncate.

…h() functions > `str_starts_with()` checks if a string begins with another string and returns a boolean value (true/false) whether it does. > `str_ends_with()` checks if a string ends with another string and returns a boolean value (true/false) whether it does. Refs: * https://wiki.php.net/rfc/add_str_starts_with_and_ends_with_functions * php/php-src#5300 * php/php-src@31fb6a0

wkhudgins92 added 3 commits March 25, 2020 17:48

Merge pull request #3 from php/master

ab7b464

update fork

Added str_starts_with and str_ends_with

a709095

Defined arg info for str_starts_with and str_ends_with

401b5a0

nikic changed the title ~~Patch add str starts with and str ends with~~ Patch add str_starts_with and str_ends_with Mar 26, 2020

nikic reviewed Mar 26, 2020

View reviewed changes

ext/standard/string.c Outdated Show resolved Hide resolved

guilliamxavier reviewed Mar 26, 2020

View reviewed changes

ext/standard/basic_functions.stub.php Outdated Show resolved Hide resolved

carusogabriel added the RFC label Mar 26, 2020

wkhudgins92 added 2 commits March 26, 2020 10:55

Fixed typo in function name

0375e0c

Added tests

21810c5

guilliamxavier reviewed Mar 26, 2020

View reviewed changes

ext/standard/string.c Outdated Show resolved Hide resolved

ext/standard/string.c Outdated Show resolved Hide resolved

ext/standard/tests/strings/str_ends_with.phpt Outdated Show resolved Hide resolved

guilliamxavier reviewed Mar 28, 2020

View reviewed changes

ext/standard/string.c Outdated Show resolved Hide resolved

wkhudgins92 added 2 commits March 30, 2020 09:16

Modernized the code some as suggested by @guilliamxavier

85c0572

Fixed typo

4e94af7

guilliamxavier reviewed Mar 30, 2020

View reviewed changes

ext/standard/tests/strings/str_ends_with.phpt Outdated Show resolved Hide resolved

Expanded tests

7ba82ca

wkhudgins92 added 3 commits March 31, 2020 15:18

Expanded null byte tests for str_starts_with and str_ends_with

9dedd4a

Reworked str_ends_with to use memcmp

321267e

Fix bug in str_ends_with

31ba825

wkhudgins92 force-pushed the patch-add-str_starts_with-and-str_ends_with branch from 0ab6f6b to 31ba825 Compare March 31, 2020 23:49

guilliamxavier reviewed Apr 1, 2020

View reviewed changes

wkhudgins92 added 2 commits April 2, 2020 20:00

fixed trailing spaces

30a9eae

Modified str_ends_with tests

4403607

wkhudgins92 added 2 commits April 2, 2020 20:36

Removed stray i from vim

0ea413f

Simplifeid str_starts_with tests

8292205

guilliamxavier reviewed Apr 3, 2020

View reviewed changes

ext/standard/tests/strings/str_ends_with.phpt Outdated Show resolved Hide resolved

guilliamxavier reviewed Apr 3, 2020

View reviewed changes

ext/standard/string.c Outdated Show resolved Hide resolved

wkhudgins92 added 4 commits April 4, 2020 12:45

changed spaces to tabs

bf63e96

Cleaned up str_ends_with tests

2509782

Merge branch 'master' into patch-add-str_starts_with-and-str_ends_with

075993f

Defined str_starts_with and str_ends_with after merge

1b42e9c

guilliamxavier reviewed Apr 5, 2020

View reviewed changes

wkhudgins92 added 2 commits April 9, 2020 14:21

Clean up string tests

303b48c

Fix str_ends_with test

52efdbf

guilliamxavier reviewed Apr 10, 2020

View reviewed changes

ext/standard/tests/strings/str_starts_with.phpt Show resolved Hide resolved

Added one more test to str_starts_with

109afd0

nikic approved these changes May 5, 2020

View reviewed changes

php-pulls closed this in 31fb6a0 May 5, 2020

carusogabriel added this to the PHP 8.0 milestone May 29, 2020

jrfnl mentioned this pull request Aug 10, 2020

PHP 8.0: NewFunctions: handle 11 new functions PHPCompatibility/PHPCompatibility#1197

Merged

Patch add str_starts_with and str_ends_with #5300

Patch add str_starts_with and str_ends_with #5300

Uh oh!

Conversation

wkhudgins92 commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glensc commented Mar 26, 2020

Uh oh!

Uh oh!

Uh oh!

guilliamxavier commented Mar 26, 2020

Uh oh!

wkhudgins92 commented Mar 26, 2020

Uh oh!

wkhudgins92 commented Mar 26, 2020

Uh oh!

guilliamxavier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wkhudgins92 commented Mar 31, 2020

Uh oh!

guilliamxavier commented Mar 31, 2020

Uh oh!

wkhudgins92 commented Apr 1, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guilliamxavier Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

wkhudgins92 Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

guilliamxavier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wkhudgins92 commented Apr 9, 2020

Uh oh!

Uh oh!

TysonAndre commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

nikic May 5, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wkhudgins92 commented Mar 25, 2020 •

edited

Loading

TysonAndre commented May 5, 2020 •

edited

Loading