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

refactor: move ArrayHelper class #8130

Merged
merged 10 commits into from
Nov 3, 2023
Merged

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Oct 31, 2023

Description

  • move ArrayHelper class to system/Helpers/Array
  • move code from array_helper.php to ArrayHelper.php

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added refactor Pull requests that refactor code 4.5 labels Oct 31, 2023
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

I wonder if this is the right place for the ArrayHelper class, but I guess it may be.

Since the removed class wasn't marked as internal, we should mention this in the changelog as a breaking change.

/**
* @see \CodeIgniter\Helpers\Array\ArrayHelperRecursiveDiffTest
*/
final class ArrayHelper
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of making this type of class final, since there might be some use cases that can benefit from extending this class. However, it's not a deal breaker for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you need it really, I will remove final.
But we can remove it anytime when request comes.

Copy link
Member

Choose a reason for hiding this comment

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

I don't feel like we need it here, but I may be the only one, so that's cool. And you're right, we can always remove it later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's wait for others opinions.

@kenjis
Copy link
Member Author

kenjis commented Oct 31, 2023

Since the removed class wasn't marked as internal, we should mention this in the changelog as a breaking change.

You mean CodeIgniter\Commands\Translation\LocalizationFinder\ArrayHelper, but it is not released.
See #7896

So I think the changelog is not needed in this case.

@michalsn
Copy link
Member

Okay, I missed that it's a new feature. In that case, no changelog entry is needed. Thanks.

I don't want to provide pubic static method.
@kenjis
Copy link
Member Author

kenjis commented Oct 31, 2023

I added @internal to ArrayHelper class.
There was discussion about changing Helper functions to classes.
But I have no intention at this time to provide helper static methods as public APIs.
If such new APIs are to be provided, more discussion is needed.
Also, I personally do not like static methods.

@kenjis kenjis merged commit abddd1d into codeigniter4:4.5 Nov 3, 2023
45 checks passed
@kenjis kenjis deleted the refactor-ArrayHelper branch November 3, 2023 00:37
@kenjis
Copy link
Member Author

kenjis commented Nov 3, 2023

If someone thinks it is better not to have final, feel free to send a PR.

@MGatner
Copy link
Member

MGatner commented Nov 7, 2023

Unless there is a compelling reason for a class not to be final we should start all new classes that way and wait for issues or requests to arise.

I am opposed to transitioning functions to public static methods just for the sake of having them in classes. If there is a case-by-case basis (like code reuse) we can look at individual files.

@michalsn
Copy link
Member

michalsn commented Nov 7, 2023

In general, I'm against static classes and using them instead of helper functions - I see no advantage at all.

When it comes to the final, I guess I'm on the opposite side. I don't like when final is used for things that potentially can be extended without any harm to the framework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.5 refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants