search

openfoodfacts / openfoodfacts-php

PHP wrapper for Open Food Facts
https://packagist.org/packages/openfoodfacts/openfoodfacts-php
MIT License
58 stars 34 forks source link

fix: ext-gd as dev. dependency #65

Open epalmans opened 2 months ago

epalmans commented 2 months ago

What

Use of ext-gd appears to only be required in tests (through imagepng() call), introduced in https://github.com/openfoodfacts/openfoodfacts-php/pull/47. Therefore, proposing to move from require to require-dev

Fixes bug(s)

sonarcloud[bot] commented 2 months ago

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

epalmans commented 2 months ago

thanks for approving.... but a bit reluctant to see it merged&tagged though due to this: https://github.com/openfoodfacts/openfoodfacts-php/pull/53#issuecomment-2159967859

Benoit382 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

@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 contact@mysuperapp.com v1.53 )

teolemon commented 1 month ago

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