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

Add DeclaredClassCasingFilter #3969

Closed
wants to merge 21 commits into from

Conversation

siad007
Copy link
Contributor

@siad007 siad007 commented Aug 12, 2018

Related to #2571

src/Fixer/Casing/NativeClassCasingFixer.php Outdated Show resolved Hide resolved
src/Fixer/Casing/NativeClassCasingFixer.php Outdated Show resolved Hide resolved
src/Fixer/Casing/NativeClassCasingFixer.php Outdated Show resolved Hide resolved
src/Fixer/Casing/NativeClassCasingFixer.php Outdated Show resolved Hide resolved
src/Fixer/Casing/NativeClassCasingFixer.php Outdated Show resolved Hide resolved
@siad007 siad007 changed the title Added NativeClassCasingFilter. Added DeclaredClassCasingFilter. Aug 18, 2018
@keradus keradus added this to the 2.14.0 milestone Aug 22, 2018
@ntzm
Copy link
Contributor

ntzm commented Sep 5, 2018

So the scope here has changed to use declared classes instead of native classes. I don't like this change because the output of the fixer can be changed depending on what classes are defined before construction, which in my case of using it as a library, it could be anything. Also, different environments will produce different results, such as:

  • I am using ext-pdo on my local environment, which defines the PDO class
  • I have some code that uses PDo
  • On my local, when I run the fixer, it fixes it correctly to PDO
  • However, on my CI setup, I might not have ext-pdo enabled, so it wouldn't fix that case

@SpacePossum
Copy link
Contributor

I don't share that concern, if you use different PHP builds/SAPI you get different results from a range of fixers already. It is also a one way fix, so on one system it fixes more than on another, but it never reverts a good fix.

@keradus keradus changed the title Added DeclaredClassCasingFilter. Add DeclaredClassCasingFilter Jan 1, 2019
@keradus keradus modified the milestones: 2.14.0, 2.15.0 Jan 1, 2019
@ntzm
Copy link
Contributor

ntzm commented Mar 18, 2019

@siad007 would you like someone to finish off this PR if you haven't got time to? It's a very useful fixer

parent::__construct();

if (null === self::$declaredClassNames) {
foreach (get_declared_classes() as $class) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sceptical about this line - get_declared_classes will not return classes that are declared, but those declared in the current script - depending how autoloader was called - which mean one day it will return some classes and another day more classes - adding some fixer to config might accidentally "discover" more classes.

When I was working on similar fixer I have ended with only internal classes to handle.

@siad007
Copy link
Contributor Author

siad007 commented Mar 18, 2019

@ntzm i have fixed the last pending issues right now

@SpacePossum
Copy link
Contributor

@siad007 thanks for the update.
I added another commit, please re-review @ntzm and @kubawerlos . Note that this fixer does not fix classes outside of the global namespace, so even if more classes are already loaded it, for example it will never fix Foo\bar, only \Foo and Foo (if needed)

],
[
'<?php
use stdclass as exception;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not be fixed to

use stdClass as exception;

?

Copy link
Contributor

Choose a reason for hiding this comment

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

@siad007 can you pick this up? also rebase and squash this, then change the target branch to master please :)

@keradus keradus modified the milestones: 2.15.0, 2.16.0 May 3, 2019
@keradus keradus changed the base branch from 2.12 to 2.15 September 4, 2019 11:01
@keradus keradus modified the milestones: 2.16.0, 2.17.0 Nov 2, 2019
@keradus keradus changed the base branch from 2.15 to 2.16 November 20, 2020 15:24
@keradus keradus modified the milestones: 2.17.0, 2.17.1, 2.18.0 Dec 7, 2020
@keradus keradus changed the base branch from 2.16 to master December 8, 2020 13:42
@keradus keradus removed this from the 2.18.0 milestone Jan 18, 2021
@SpacePossum
Copy link
Contributor

Thanks @siad007 for all the work here!

The feature has been added to the code base here #6262 , with big difference that it only fixes internal class names and addresses some other feedback items from here, thanks all 👍

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

5 participants