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

Expand docblock of JoinSub to allow other models #1352

Merged

Conversation

harmenjanssen
Copy link
Contributor

@harmenjanssen harmenjanssen commented Sep 1, 2022

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Expands on #1212.

Changes

The previous version of this docblock used the template TModelClass.
However, this template was already in use at the class level, effectively limiting the argument to joinSub to be a QueryBuilder of the same type.

The test proved that this was allowed:

User::query()->joinSub(
    User::query()->whereIn('id', [1, 2, 3]),
    'users',
    'users.id',
    'users.id'
);

However, in the wild, I noticed that User::query()->joinSub(Post::query(), ...) was not allowed.

This change fixes that, simply by renaming the template in the joinSub docblock.

Breaking changes

None.

The previous version of this docblock used the template `TModelClass`.
However, this template was already in use at the class level,
effectively limiting the argument to `joinSub` to be a QueryBuilder of
the same type.

The test proved that this was allowed:

```
User::query()->joinSub(
    User::query()->whereIn('id', [1, 2, 3]),
    'users',
    'users.id',
    'users.id'
);
```

However, in the wild, I noticed that
`User::query()->joinSub(Post::query(), ...)` was _not_ allowed.

This change fixes that, simply by renaming the template in the `joinSub`
docblock.
@harmenjanssen harmenjanssen marked this pull request as ready for review September 1, 2022 05:07
@harmenjanssen
Copy link
Contributor Author

Hi!
Is there an interest in merging this PR? I'm wondering if I can do anything to remove any blocking factors, if not.
Please let me know if this needs an update or a change!

@szepeviktor
Copy link
Collaborator

Oh! So you want to free joinSub.

@szepeviktor
Copy link
Collaborator

I'd merge it after having a quick line in the changelog.

@harmenjanssen
Copy link
Contributor Author

Sure thing! Do you want me to add a new section in the Changelog for 2.2.1?

@szepeviktor
Copy link
Collaborator

In the Unreleased section.

@harmenjanssen
Copy link
Contributor Author

Done!

@szepeviktor
Copy link
Collaborator

I like your dashes.

@szepeviktor szepeviktor merged commit dd1087d into larastan:master Sep 26, 2022
@szepeviktor
Copy link
Collaborator

Thank you!

@harmenjanssen harmenjanssen deleted the allow-other-models-in-joinsub branch September 26, 2022 08:20
@harmenjanssen
Copy link
Contributor Author

Oops. 😅 Glad you like them, that was Prettier auto-formatting the file.

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.

None yet

2 participants