Skip to content

Conversation

kubatek94
Copy link
Contributor

Overview

  • I have performed a self review of my code
  • I have made corresponding changes to the documentation
  • I have commented my code, particularly in hard-to-understand areas
  • I have added sufficient logging
  • New and existing tests pass locally with my changes
  • The code modified as part of this PR has been covered with tests
  • The API spec has been modified inline with the changes in this PR

Changes

  1. Dropped support for PHP 8.0 and Laravel 8
  2. Added support for PHP 8.2 and Laravel 10
  3. Added Netsells code standards to dependencies and github action workflow to check the code style and perform static analysis
  4. Fixes / additions of few PHP docs after running phpstan
  5. Code style changes to be compatible with our code standards

Dependencies

Test procedures

Links

JIRA: https://netsells.atlassian.net/browse/

Deployment

Migrations

Yes/No

Environment variables

Deployment notes

@kubatek94 kubatek94 marked this pull request as ready for review April 14, 2023 15:42
@kubatek94 kubatek94 requested a review from tomchkk April 14, 2023 15:42
{
static::loadEloquentRelations($deferredValues);

collect($deferredValues)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is collect() deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not and is still available. I thought that it would be nice not to depend on a global function included from the Laravel helpers file, so I switched to using Collection class directly. However it was a bit of a 'half assed' attempt because there are still calls to data_get (which is only available as such global helper, and not a class I can hook into). So all in all, not a massive improvement and probably a bit of a useless change :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Does Illuminate\Support\Arr::get() not cut the mustard?

Copy link
Contributor Author

@kubatek94 kubatek94 Apr 18, 2023

Choose a reason for hiding this comment

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

Hard to say. Arr::get passes the tests, but looking at the implementation of both, I am not sure if Arr::get covers all the use cases that data_get does. The 'relation' string is controlled by the users of this package, so I can't be sure that it never gets called with e.g. '*' which is valid segment for data_get but not for Arr:get. In theory it shouldn't be, and the only supported 'relation' strings should be compatible with the Laravel Eloquent relation syntax. I think this change would require a new major version of this package just in case, and then I would also like to look at improved handling of the relation string in general - so would prefer to leave it as is, and then do the switch to Arr::get as part of this work. However, thanks for throwing that excellent suggestion, as I would definitely want to switch to that in the future! :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, they do look a little different.

@kubatek94 kubatek94 requested a review from tomchkk April 18, 2023 08:57
@kubatek94 kubatek94 merged commit 6e083d3 into master May 15, 2023
@kubatek94 kubatek94 deleted the feature/php8.2 branch May 15, 2023 08:44
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.

2 participants