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

NoAliasFunctionsFixer - Add more function aliases #5588

Closed
wants to merge 5 commits into from

Conversation

danog
Copy link
Contributor

@danog danog commented Apr 1, 2021

Extracted directly from the PHP trunk (master&7.4).

@coveralls
Copy link

coveralls commented Apr 1, 2021

Coverage Status

Coverage decreased (-0.004%) to 92.172% when pulling ae5a964 on zoonru:2.18 into 2963845 on FriendsOfPHP:master.

@danog
Copy link
Contributor Author

danog commented Apr 1, 2021

Note quite sure what to make of the phpstan analysis, seems like a bug to me (mixed is a supertype of string; plus the error is not triggered on other lines of the elseif))
As a friendly recommendation from a psalm lover, I can really recommend switching to https://psalm.dev for static analysis :)

@kubawerlos
Copy link
Contributor

That's weird, I've reported a bug: phpstan/phpstan#4793 - check out the example, it's really strange.

As workaround you can add above the line with foreach ($this->configuration['sets'] as $set) { PHPdoc like this: /** @var string $set */

I can really recommend switching to https://psalm.dev

Why not both? I often use both as the can detect different issues and both have some bugs.

@GrahamCampbell
Copy link
Contributor

Needs tests covering some of these options.

@keradus keradus changed the base branch from 2.18 to 2.19 May 3, 2021 22:09
@SpacePossum
Copy link
Contributor

Thanks @danog for these improvements :)

Please add the some tests here and have a look at my minors, looking close to finished

@SpacePossum SpacePossum added this to the 2.19.1 milestone May 25, 2021
@SpacePossum SpacePossum changed the title Add more function aliases NoAliasFunctionsFixer - Add more function aliases May 25, 2021
@danog
Copy link
Contributor Author

danog commented Jun 22, 2021

Finally got around to finishing this MR :)
Applied the requested changes, and added some tests based on the existing test logic.
The only detail that I'm not sure about is the @pg set: the only alias function contained in that set is the extremely old pg_exec alias, deprecated way back in PHP 4.
I'm not sure if it's worth removing from the defaults.

@danog
Copy link
Contributor Author

danog commented Jul 26, 2021

All OK over here? :)

@SpacePossum SpacePossum modified the milestones: 2.19.2, 2.19.1 Jul 28, 2021
@SpacePossum SpacePossum added the RTM Ready To Merge label Jul 28, 2021
@keradus keradus removed the RTM Ready To Merge label Jul 29, 2021
@keradus
Copy link
Member

keradus commented Jul 29, 2021

@danog can you rebase on 2.19 or allow to modify your branch by us? we need to ensure that CI kicks off for this PR and all jobs to be green, before we are able to merge.

@keradus
Copy link
Member

keradus commented Jul 29, 2021

I don't see it solving any bug in the existing fixer. it's giving existing fixer more functionality (to handle more aliases)

should go to 3.1 line

@keradus keradus removed this from the 2.19.1 milestone Jul 29, 2021
@keradus keradus added this to the 3.1.0 milestone Jul 29, 2021
@keradus keradus changed the base branch from 2.19 to master July 29, 2021 22:58
@danog
Copy link
Contributor Author

danog commented Jul 30, 2021

There, all fixed up nicely.

@keradus
Copy link
Member

keradus commented Aug 2, 2021

Thank you @danog.

keradus added a commit that referenced this pull request Aug 2, 2021
This PR was squashed before being merged into the master branch.

Discussion
----------

NoAliasFunctionsFixer - Add more function aliases

Extracted directly from the PHP trunk (master&7.4).

Commits
-------

3d3e267 NoAliasFunctionsFixer - Add more function aliases
@keradus keradus closed this Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants