if a tag does not exist, we dont want the query to crash

[tom-it]

No description provided.

[tom-it]

I'm currently working on a flarum implementation, and noticed the tags search would fail when a tag was entered that did not exist.

this works:
https://discuss.flarum.org/?q=tag%3Asupport%20tag%3Ainstallation

this does not work:
https://discuss.flarum.org/?q=tag%3Asupport%20tag%3Ainstallationabc

this pull request will fix the query to return an empty result

[tom-it]

not sure why one test failed..

[luceos]

Because your conditional markup is incorrect, see: https://github.styleci.io/analyses/zdBJZv

[tom-it]

Strange, looks fine to me in the commit 6680e5a

[clarkwinkelmann]

@tom-it it's missing the space between if (. Also == true is unnecessary as isEmpty() returns a boolean already 😉

Also, indentation is wrong. I think StyleCI gives you the patch to make the necessary changes.

[tom-it]

@tom-it it's missing the space between if (. Also == true is unnecessary as isEmpty() returns a boolean already 😉

Also, indentation is wrong. I think StyleCI gives you the patch to make the necessary changes.

Ah Thanks, will check it out!

adding the == true is a personal preference, but I understand that the code needs to be uniform to prevent different styles from cluttering up the codebase.

[luceos]

I was looking at this PR and - although solving it is absolutely a requirement - the approach is off.

The problem here is that the repository method getIdForSlug is returning a Collection instance, instead it should return an int as also shown in the phpdoc of that method. This is a result of an upgrade to Laravel 5.7 components a while ago and possibly a bug from before that.

I'd like to propose the following solution and hoping you, @tom-it, would like to give this a shot:

• Update the getIdForSlug with return type hinting : ?int so that the method will always error when something different than an integer or null is returned.
• Update the getIdForSlug to do the isEmpty check, return null if so, otherwise return the first result and its id property. You could potentially use firstOrNew so that you get an empty object if no entry exists, that would make id null or int by default (I think).

Thanks for your first ever PR to this project!

[tom-it]

I was looking at this PR and - although solving it is absolutely a requirement - the approach is off.

The problem here is that the repository method getIdForSlug is returning a Collection instance, instead it should return an int as also shown in the phpdoc of that method. This is a result of an upgrade to Laravel 5.7 components a while ago and possibly a bug from before that.

I'd like to propose the following solution and hoping you, @tom-it, would like to give this a shot:

• Update the getIdForSlug with return type hinting : ?int so that the method will always error when something different than an integer or null is returned.
• Update the getIdForSlug to do the isEmpty check, return null if so, otherwise return the first result and its id property. You could potentially use firstOrNew so that you get an empty object if no entry exists, that would make id null or int by default (I think).

Thanks for your first ever PR to this project!

Sure thing, I will check it out tonight.

I'm currently working on a flarum project so I will be doing some more PR's on the main flarum project as well, and see where I can help out on any open issues.

[luceos]

That would be great Thomas and I love seeing another 🇳🇱around here ;)

If you have any questions, we're on discord most of the time http://flarum.org/discord/

[tom-it]

That would be great Thomas and I love seeing another 🇳🇱around here ;)

If you have any questions, we're on discord most of the time http://flarum.org/discord/

I have added the functionality as described, still styleci doesn't like my commit, do I need to remove the new lines?

[tom-it]

Let me know if you need anything else for this.

[tobyzerner]

Looks good, thanks @tom-it. And the original issue is still fixed?

[tom-it]

@tobyzerner correct tested and working

[luceos]

The original issue is also guaranteed to be fixed because of the return type hinting. A Collection instance would trigger an error.

Great job Thomas!