Skip to content

Add clearer guidance for filter return values - #521

Merged
dgw merged 4 commits into
mainfrom
filter-shunt-value-guidance
Aug 18, 2026
Merged

Add clearer guidance for filter return values#521
dgw merged 4 commits into
mainfrom
filter-shunt-value-guidance

Conversation

@dgw

@dgw dgw commented Aug 18, 2026

Copy link
Copy Markdown
Member

Pre-requisites

Change Description

This is my attempt at addressing @ozh's documentation suggestion in YOURLS/dont-log-bots#5 (comment)

I was trying to get it done fairly quickly in between things IRL, so I'm sure there's room for improvement. But the important thing is having an open PR at all, reminding us to address the documentation gap. 🙂

Some shunt-using plugins out there return values like `false` to the
shunt filter if their conditions aren't met. This used to work, but was
never "correct" according to the API contract.

We should have had clearer guidance on this sooner, though.
@dgw
dgw requested a review from a team as a code owner August 18, 2026 17:18
@dgw dgw added the documentation Improvements or additions to documentation label Aug 18, 2026
@dgw

dgw commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

@LeoColomb The build succeeded when the lint was failing, and now the build is failing after linter auto-fixes (to text I didn't touch, fwiw). Would appreciate you taking a look when you have a minute!

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
website a6649af Commit Preview URL

Branch Preview URL
Aug 18 2026, 07:43 PM

Comment thread docs/development/plugins.md Outdated
Comment thread docs/development/plugins.md Outdated
Co-authored-by: Léo Colombaro <LeoColomb@users.noreply.github.com>
@dgw
dgw merged commit b43394c into main Aug 18, 2026
22 checks passed
@dgw
dgw deleted the filter-shunt-value-guidance branch August 18, 2026 19:51
@ozh

ozh commented Aug 19, 2026

Copy link
Copy Markdown
Member

Hey,

a bit late into the conversation -- thanks @dgw for proposing this documentation improvement

I have a suggestion for the shunt code sample: instead of

yourls_add_filter('shunt_update_clicks', 'yp_dont_log_conditional');
yourls_add_filter('shunt_log_redirect', 'yp_dont_log_conditional');

function yp_dont_log_conditional($value = yourls_shunt_default()) {
    if (yp_dont_log_conditions_match()) {
        return true;
    }
    return $value;
}

simply

yourls_add_filter('shunt_update_clicks', 'yp_dont_log_conditional');
yourls_add_filter('shunt_log_redirect', 'yp_dont_log_conditional');

function yp_dont_log_conditional( $dont_shunt ) {
    if (yp_dont_log_conditions_match()) {
        return true;
    }
    return $dont_shunt;
}

Bigger improvement: the function doesn't rely on 1.10.4+ yourls_shunt_default() so it's fully compatible with older installs. Shunt filter function don't need a default value since they are guaranteed to get a defined value.

Little improvement: naming the param $dont_shunt might make it more explicit that if conditions are not met in your filter function, you return something that does not shunt.

Thoughts ?

@dgw

dgw commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

Good point about <1.10.4, and not needing a default value.

How about $original_value for the param name? I think it'd flow nicer in the prose that mentions it than trying to word around the name $dont_shunt.

Lemme open another PR with these tweaks as separate commits and we can continue there.

@ozh

ozh commented Aug 19, 2026

Copy link
Copy Markdown
Member

Yes $original_value is maybe nicer

@dgw dgw mentioned this pull request Aug 19, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants