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

fix: ext-gd as dev. dependency #65

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

fix: ext-gd as dev. dependency #65

wants to merge 2 commits into from

Conversation

epalmans
Copy link
Member

What

Use of ext-gd appears to only be required in tests (through imagepng() call), introduced in #47. Therefore, proposing to move from require to require-dev

Fixes bug(s)

@epalmans epalmans requested a review from a team as a code owner June 11, 2024 05:31
@epalmans epalmans changed the title ext-gd as dev. dependency fix: ext-gd as dev. dependency Jun 11, 2024
Copy link

sonarcloud bot commented Jun 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@teolemon teolemon left a comment

Choose a reason for hiding this comment

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

Thanks @epalmans 👌

@epalmans
Copy link
Member Author

thanks for approving.... but a bit reluctant to see it merged&tagged though due to this: #53 (comment)

@Benoit382
Copy link
Collaborator

Benoit382 commented Jul 11, 2024

Add the user-agent as first parameter help us to force developer to add it, if it was the last, it will never used.
You can create a 0.4.0 version.

--
@teolemon what did you think ?
Force developer to add user agent (but it could use the same) or move it to last parameter with default value (it will be probably the same for all developer)

@Dwarfex
Copy link
Member

Dwarfex commented Jul 12, 2024

@Benoit382
Just my two cents: if you change the order of the parameters - you break every implementation using the current library and to follow semantic versioning - you might consider doing this only with a new major release.
(I know it is still on a 0.x version - but this is not really a nice way of changing things)

Every code using this library will then have to adapt - please consider - also the laravel plugin of @epalmans

@epalmans
Copy link
Member Author

another thought... we could keep the constructor like it was before, but now check for the User-Agent request header. Then, if it's missing, return a 403 with an errormessage?

@teolemon
Copy link
Member

@Benoit382 I don't have massive opinions. We proposed that as we were overwhelmed with queries.
I would assume that a developer has fixed/pinned dependancies, and will spot the issue on updating as the SDK will spit out an error in his/her console or the CI. That will entice them to provide some info about his/her app/service (or worst case put bogus info)
We should definitely provide a easy to apply sample that takes 1 min to adapt (MySuperApp [email protected] v1.53 )

@teolemon
Copy link
Member

(and sorry for the delay, I was in holidays last week)

@teolemon teolemon added the dependencies Pull requests that update a dependency file label Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

4 participants