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

Constructive bound checking on RGB/RGBA values #74

Closed
jcogs-design opened this issue May 14, 2022 · 1 comment
Closed

Constructive bound checking on RGB/RGBA values #74

jcogs-design opened this issue May 14, 2022 · 1 comment
Assignees

Comments

@jcogs-design
Copy link
Contributor

I have developed a modification for RGB and RGBA that accepts parameter values for R G B as %, as per spec for these color definitions - so for example making this possible:

$rgb = Rgb::fromString('rgb(30%, 50%, 70%)');
echo $rgb; // rgb(77,128,179)

In doing this I have (through force of habit) added boundary checks that coerce the values created from strings to sit within the range 0-255 (or 0-1 for opacity) - using simple min/max testing.

I went to write some tests for these adapted fromString conversions I noticed that the current tests are set to throw an exception if RGB values exceed boundaries. I understand why this might be done, but in general I think it is an unforgiving way of operating - it is kinder / more constructive to the developer to return a boundary value when offered value exceeds the boundary than to kill the php process - so if an alpha value of 1.5 is specified interpret this as 1.

I would value some policy guidance on whether this kind of constructive boundary checking is allowed or not before I submit the PR with the adapted code: hopefully this will save time compared to submitting the PR and then having to resubmit when constructive boundary checking is deemed inappropriate.

Look forward to hearing thoughts etc.

@spatie-bot
Copy link

Dear contributor,

because this issue seems to be inactive for quite some time now, I've automatically closed it. If you feel this issue deserves some attention from my human colleagues feel free to reopen it.

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

No branches or pull requests

3 participants