Skip to content

Add private types on 4.x#4218

Merged
fabpot merged 1 commit into
twigphp:4.xfrom
smnandre:feat/add-private-types
Aug 21, 2024
Merged

Add private types on 4.x#4218
fabpot merged 1 commit into
twigphp:4.xfrom
smnandre:feat/add-private-types

Conversation

@smnandre

@smnandre smnandre commented Aug 20, 2024

Copy link
Copy Markdown
Contributor

(WIP but i open to gather early feedback / comments)

Objective: add missing types

  • Private properties
  • Return type on private methods
  • Argument types on private methods (?not sure)

Maybe "@internal" then

@smnandre

Copy link
Copy Markdown
Contributor Author

Remaining non-typed arguments inherit their "non-types" from protected or public methods, not sure what to do there

Example in Profiler\Dumper\BaseDumper

    abstract protected function formatTime(Profile $profile, $percent): string;

    private function dumpProfile(Profile $profile, $prefix = '', $sibling = false): string

Comment thread src/ExpressionParser.php Outdated
Comment thread src/NodeVisitor/EscaperNodeVisitor.php Outdated
@stof

stof commented Aug 21, 2024

Copy link
Copy Markdown
Member

Actually, types on all private APIs could be added in the 3.x branch

@smnandre

Copy link
Copy Markdown
Contributor Author

So i should target 3.x for this one @stof right ? And then deal with @internal for 4.x ?

@smnandre smnandre force-pushed the feat/add-private-types branch from ceaf8ea to 1864c38 Compare August 21, 2024 17:53
@smnandre

Copy link
Copy Markdown
Contributor Author

(is there something like a "fabbot_ignore"? to prevent false-positive ? asking more generally than for this PR)

@smnandre smnandre requested a review from fabpot August 21, 2024 17:54
@fabpot

fabpot commented Aug 21, 2024

Copy link
Copy Markdown
Contributor

Let's not bother with 3.x.

@smnandre

Copy link
Copy Markdown
Contributor Author

So this seems ok for a first step.

Let's create another one for @internal function/methods if you're ok ?

    /**
     * Strips HTML and PHP tags from a string.
     *
     * @param string|null          $string
     * @param string[]|string|null $allowable_tags
     *
     * @internal
     */
    public static function striptags($string, $allowable_tags = null): string
    {
        return strip_tags($string ?? '', $allowable_tags);
    }

Because here there may be a "soft-conflict" between the signature (string|null) and the code (passing null to strip_tags is deprecated since 8.1)

@fabpot fabpot force-pushed the feat/add-private-types branch from 1864c38 to 72bbe48 Compare August 21, 2024 18:05
@fabpot

fabpot commented Aug 21, 2024

Copy link
Copy Markdown
Contributor

Thank you @smnandre.

@fabpot fabpot merged commit 96e5d65 into twigphp:4.x Aug 21, 2024
@stof

stof commented Aug 21, 2024

Copy link
Copy Markdown
Member

@smnandre there is no conflict in your code snippet. The code is not passing null to strip_tags

@smnandre

Copy link
Copy Markdown
Contributor Author

Wow... let's just attribute it to today's emotions. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants