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

Copy Debug class from doctrine/common #11000

Merged
merged 1 commit into from
Oct 14, 2023
Merged

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Oct 12, 2023

This is an alternative to 4c8934f (the other commit in the associated PR still stands).

This reduces our dependency to this shared library that now holds very little code we use.
The class has not been copied verbatim:

  • Unused parameters and methods have been removed.
  • The class is final and internal.
  • Coding standards have been enforced, including enabling strict_types, which lead to casting a variable to string before feeding it to explode().
  • A bug found by static analysis has been addressed, where an INI setting obtained with ini_get() was compared with true, which is never returned by that function.

A follow-up of this PR could be to leverage Symfony's VarDumper component when available.

@greg0ire greg0ire force-pushed the copy-debug branch 3 times, most recently from 62c1203 to d15fbed Compare October 12, 2023 12:09
*
* @dataProvider provideAttributesCases
*
* @requires PHP < 8.1.0
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently, the representation differs. I can make the data provider output different orders based on the PHP version so that we continue testing this on 3.0

Copy link
Member

Choose a reason for hiding this comment

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

Do the representations only differ in the order of the array keys? I think PHP 8 changed the order in which parent and child properties are dumped.

If that's the case, let's put the PHP 8.1 representation into the data provider and if we're on PHP 7, we ksort() both arrays before comparing them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's the only difference. I'll try what you are recommending.

Copy link
Member Author

@greg0ire greg0ire Oct 12, 2023

Choose a reason for hiding this comment

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

ksort does not do the trick the order seems to be alphabetical, then public, protected, private :P: https://github.com/doctrine/orm/actions/runs/6500148405/job/17654937509?pr=11000#step:5:89

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's try that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually that won't work: we're comparing strings (generated by print_r) here, not arrays. Besides, on 3.0.x, this canonicalization will be useless. As it stands, the code makes it more clear that something can be removed.

Copy link
Member

Choose a reason for hiding this comment

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

All right.

@greg0ire greg0ire requested a review from derrabus October 12, 2023 18:39
@greg0ire greg0ire force-pushed the copy-debug branch 3 times, most recently from 190df5f to 63ce659 Compare October 12, 2023 19:46
@derrabus derrabus added this to the 2.17.0 milestone Oct 12, 2023
derrabus
derrabus previously approved these changes Oct 12, 2023
lib/Doctrine/ORM/Tools/Debug.php Show resolved Hide resolved
lib/Doctrine/ORM/Tools/Debug.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/Tools/Debug.php Show resolved Hide resolved
lib/Doctrine/ORM/Tools/Debug.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/Tools/Debug.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/Tools/Debug.php Outdated Show resolved Hide resolved
@greg0ire greg0ire marked this pull request as draft October 13, 2023 09:20
This reduces our dependency to this shared library that now holds very
little code we use.
The class has not been copied verbatim:
- Unused parameters and methods have been removed.
- The class is final and internal.
- Coding standards have been enforced, including enabling strict_types,
  which lead to casting a variable to string before feeding it to
  explode().
- A bug found by static analysis has been addressed, where an INI
  setting obtained with ini_get() was compared with true, which is never
  returned by that function.
- Tests are improved to run on all PHP versions
@greg0ire greg0ire requested a review from derrabus October 14, 2023 19:28
@greg0ire greg0ire marked this pull request as ready for review October 14, 2023 19:28
@greg0ire greg0ire merged commit edd962e into doctrine:2.17.x Oct 14, 2023
58 checks passed
@greg0ire greg0ire deleted the copy-debug branch October 14, 2023 21:00
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.

3 participants