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

[cs] introduce modern string functions #366

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

connorhu
Copy link
Collaborator

@connorhu connorhu commented Apr 4, 2024

This PR is introduce the str_starts_with str_ends_with str_contains functions where is possible.
It improves code readability and also php8 or above cause an improvement in performance.

Mostly automated fixes are done using Rector and php-cs-fixer. Relevant rector rules:

  • \Rector\Php80\Rector\Identical\StrEndsWithRector
  • \Rector\Php80\Rector\NotIdentical\StrContainsRector
  • \Rector\Php80\Rector\Identical\StrStartsWithRector

For php 7.4 symfony/polyfill-php80 provides backwards compatibility.

Template for rector:

<?php

use Rector\Config\RectorConfig;

return static function (RectorConfig $rectorConfig): void {
    $rectorConfig->paths([
        __DIR__.'/lib',
        __DIR__.'/test',
    ]);
    $rectorConfig->skip([
        __DIR__.'/lib/*/skeleton/*',
    ]);

@connorhu
Copy link
Collaborator Author

connorhu commented Apr 4, 2024

A skeleton jumped out of the closet. The problem is that the current codebase doesn't recognize and use the composer autoloader, everyone has to load it themselves (see migration wiki).

Copy link
Contributor

@alquerci alquerci left a comment

Choose a reason for hiding this comment

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

I am not sure.

Indeed adding new dependency that requires the use of composer is a BC break.

As composer is not yet required.

@connorhu
Copy link
Collaborator Author

connorhu commented Apr 5, 2024

I am not sure.

Indeed adding new dependency that requires the use of composer is a BC break.

As composer is not yet required.

Yes, this is a valid point. This is indeed a BC break in the sense that there was no composer req. before (although we support it). I don't see this as a problem in the sense that composer is no cutting edge today and this change is more about bringing practices that have become standard.

On the other hand, due to limited maintenance resources, I would also remove PEAR and the now-defunct symfony plugin installer.

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.

2 participants