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

[libraries/Utility] Add startsWith, endsWith string functions to Utility (with example) #2965

Closed
wants to merge 1 commit into from
Closed

[libraries/Utility] Add startsWith, endsWith string functions to Utility (with example) #2965

wants to merge 1 commit into from

Conversation

johnsaigle
Copy link
Contributor

@johnsaigle johnsaigle commented Jul 19, 2017

  • Create syntax sugar for substr to prevent developer errors with string comparison

  • Add startsWith string comparison function to Utility

  • Add endsWith string comparison function to Utility

  • see [Help Editor] Fix incorrect substr syntax #2966 for an example of this error

@johnsaigle johnsaigle added the Category: Bug PR or issue that aims to report or fix a bug label Jul 19, 2017
@johnsaigle johnsaigle added this to the 17.1 milestone Jul 19, 2017
@johnsaigle johnsaigle changed the title [Utility] Add startsWith, endsWith string functions to Utility (with example) [Utility] Add startsWith, endsWith string functions to Utility (with example bugfix) Jul 19, 2017
@johnsaigle johnsaigle added the Category: Feature PR or issue that aims to introduce a new feature label Jul 19, 2017
@johnsaigle johnsaigle changed the title [Utility] Add startsWith, endsWith string functions to Utility (with example bugfix) [libraries/Utility] Add startsWith, endsWith string functions to Utility (with example bugfix) Jul 19, 2017
@johnsaigle johnsaigle modified the milestones: 17.2, 17.1 Jul 19, 2017
@johnsaigle johnsaigle changed the title [libraries/Utility] Add startsWith, endsWith string functions to Utility (with example bugfix) [libraries/Utility] Add startsWith, endsWith string functions to Utility (with example) Jul 19, 2017
@driusan driusan removed this from the 18.1 milestone Oct 2, 2017
@ridz1208 ridz1208 changed the base branch from 17.1-dev to minor October 21, 2017 17:29
static function endsWith($haystack, $needle)
{
return substr($haystack, -strlen($needle)) === $needle;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think these would make more sense in a new Strings class, rather than adding more stuff to the already overloaded Utility class.

@ridz1208
Copy link
Collaborator

@johnsaigle @driusan I think the current way these are implemented is problematic for a couple reasons:

  • with the current hectic state of the utility class, these functions will get lost in the stack and noone will use someone goes and replaces all substr calls to use these.. I'm scared they will find their way yo a PR like [Libraries] Remove unused functions from Utility and update cleanup comments #2913 very very quickly

  • I think its fair to expect a developer to know how to find/read documentation on php function usage... personally I feel like this will make developers more lazy !!

I wouldn't be opposed to create a new String.class.inc and have those there (they will be easier to find) the same PR should have functions such as contains(needle,haystack) and postion(needle,haystack) But i would insist on these being used multiple places in the code before the PR is merged

@ridz1208 ridz1208 added State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) and removed Category: Bug PR or issue that aims to report or fix a bug labels Nov 23, 2017
@johnsaigle
Copy link
Contributor Author

@ridz1208 I am in favour of a Strings class to make things simpler. I added these not so much because of new developers but because of PHP being pretty inconsistent.

In fact I added them because of #2953. Misuse of strpos caused some critical security issues and (not that anybody is keeping track) but I believe these functions were written by Dave who is by no means a jr PHP dev. My point is not to cast blame but just to say that everyone makes mistakes sometimes.

Given that a lot of code is copy-pasted as people learn how to e.g. write modules I think having some little utility functions like this help improve code readability and can help you think more clearly while writing -- and the alternative is keeping track of every little detail of PHP weirdness which is pretty tough.

@ridz1208
Copy link
Collaborator

@johnsaigle I don't make the distinction between new/old and jr/sr developers... everyone makes mistakes, yes including dave !!

But Dave's code got reviewed and merged so there is at least one other developer who also failed to catch the error. The issue in my opinion is not PHP being inconsistent (even if that does not help) but improper management of PRs which is an issue we have been tackling lately and I think we made significand improvements.

Tha said, I'm not THAT opposed to these chnages, it is an improvement... will keep this PR open for further development

@ridz1208 ridz1208 added the State: Needs work PR awaiting additional work by the author to proceed label Jan 15, 2018
add starts with and endswith to Utility

add example

phpcs
@johnsaigle
Copy link
Contributor Author

Now that I look this over ain, I don't know that this is actually fixing the problem. Someone unfamiliar with the code would still need to read this over to see if it's $needle, $haystack or $haystack, $needle.

@johnsaigle johnsaigle closed this Apr 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Feature PR or issue that aims to introduce a new feature State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) State: Needs work PR awaiting additional work by the author to proceed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants