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

Created a positiblity to add custom relation type #987

Merged
merged 9 commits into from
Sep 8, 2020
Merged

Created a positiblity to add custom relation type #987

merged 9 commits into from
Sep 8, 2020

Conversation

efinder2
Copy link
Contributor

@efinder2 efinder2 commented Jul 8, 2020

In a project of mine I use the eloquent-has-many-deep library. To enable the ide-helper for it, I wrote a possibility to add a config option to add custom relation types.

I know the code isn't tested yet. I'll add it as soon as the idea of the pull request has been discussed.

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

IMHO looks good to move forward with tests

Thanks :)

src/Console/ModelsCommand.php Outdated Show resolved Hide resolved
src/Console/ModelsCommand.php Outdated Show resolved Hide resolved
@mr-feek
Copy link
Contributor

mr-feek commented Jul 10, 2020

Is this something we can/should solve via reflection? Perhaps that is difficult, though. We would need to search through each method, and see if the return type is a subtype of Relation. What do we think? I might be willing to take a stab at it!

@efinder2
Copy link
Contributor Author

It might not be the perfect solution at the moment. Not all code uses a strict return type. The linked library for example hasn't implemented them yet, so reflection might be a great addition to get rid of the mapping for types methods. As fallback I suggest to keep the config option

@mfn
Copy link
Collaborator

mfn commented Jul 10, 2020

Personally I would reduce the time invested, the config approach seems a pragmatic approach. I bet it doesn't come up often. First time I hear about this requirement in years 😄

@efinder2
Copy link
Contributor Author

I've looked into the source from Laravel. The relations don't have a return type. So the reflection would need Parsing of the return type of the doc block. This isn't ideal.

@efinder2
Copy link
Contributor Author

efinder2 commented Jul 10, 2020

Only Problem I see in the config approach, may be two libraries modifying the config. For future extension it might be helpful to create a HelperClass for adding class mappings to the config property.
I've also added a test for the change.

Please review the Test. I'm not sure everything is covert.

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

@barryvdh LGTM!

src/Console/ModelsCommand.php Show resolved Hide resolved
config/ide-helper.php Outdated Show resolved Hide resolved
mfn added 3 commits September 7, 2020 21:10
# Conflicts:
#	config/ide-helper.php
#	src/Console/ModelsCommand.php
#	tests/Console/ModelsCommand/Relations/Test.php
Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

@efinder2 I took the liberty and updated the PR with master, applied my own feedback and added a changelog entry

@barryvdh IMHO this is good to merge!

@barryvdh
Copy link
Owner

barryvdh commented Sep 7, 2020

Hmm it has conflicts again

# Conflicts:
#	CHANGELOG.md
@mfn
Copy link
Collaborator

mfn commented Sep 7, 2020

@barryvdh fixed!

We'll likely see this more often now with the changelog, adding entries at the top I fear…

@barryvdh barryvdh merged commit 7305504 into barryvdh:master Sep 8, 2020
fatihdirikman added a commit to fatihdirikman/Laravel-IDE-Helper that referenced this pull request Jan 7, 2022
* Created a positiblity to add custom relation type

* Changed access modifier

Co-authored-by: Markus Podar <[email protected]>

* fixed hints from code review

* Added Test for barryvdh/laravel-ide-helper#987

* composer fix-style

* Clarify doc in config entry

* Update CHANGELOG.md

Co-authored-by: Markus Podar <[email protected]>
renaforsberg824 added a commit to renaforsberg824/ide-helper-laravel-developer that referenced this pull request Oct 5, 2022
* Created a positiblity to add custom relation type

* Changed access modifier

Co-authored-by: Markus Podar <[email protected]>

* fixed hints from code review

* Added Test for barryvdh/laravel-ide-helper#987

* composer fix-style

* Clarify doc in config entry

* Update CHANGELOG.md

Co-authored-by: Markus Podar <[email protected]>
lisadeloach63 added a commit to lisadeloach63/ide-helper-reso-laravel that referenced this pull request Oct 7, 2022
* Created a positiblity to add custom relation type

* Changed access modifier

Co-authored-by: Markus Podar <[email protected]>

* fixed hints from code review

* Added Test for barryvdh/laravel-ide-helper#987

* composer fix-style

* Clarify doc in config entry

* Update CHANGELOG.md

Co-authored-by: Markus Podar <[email protected]>
sadafrangian3 pushed a commit to sadafrangian3/ide-helper-laravel that referenced this pull request Oct 18, 2022
* Created a positiblity to add custom relation type

* Changed access modifier

Co-authored-by: Markus Podar <[email protected]>

* fixed hints from code review

* Added Test for barryvdh/laravel-ide-helper#987

* composer fix-style

* Clarify doc in config entry

* Update CHANGELOG.md

Co-authored-by: Markus Podar <[email protected]>
smile1130 added a commit to smile1130/laravel-IDE that referenced this pull request Jun 16, 2023
* Created a positiblity to add custom relation type

* Changed access modifier

Co-authored-by: Markus Podar <[email protected]>

* fixed hints from code review

* Added Test for barryvdh/laravel-ide-helper#987

* composer fix-style

* Clarify doc in config entry

* Update CHANGELOG.md

Co-authored-by: Markus Podar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants