Skip to content

Commit

Permalink
Fix #318, these macros are not used as planed, we have separate funct…
Browse files Browse the repository at this point in the history 
…ions for each
  • Loading branch information
@pierrejoye
pierrejoye committed Aug 25, 2021
1 parent a24e96f commit bdc281e
Showing 1 changed file with 0 additions and 5 deletions.
5 changes: 0 additions & 5 deletions src/gd.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1604,11 +1604,6 @@ BGD_DECLARE(void) gdImageFlipHorizontal(gdImagePtr im);
BGD_DECLARE(void) gdImageFlipVertical(gdImagePtr im);
BGD_DECLARE(void) gdImageFlipBoth(gdImagePtr im);

#define GD_FLIP_HORINZONTAL 1 /* typo, kept for BC */
#define GD_FLIP_HORIZONTAL 1
#define GD_FLIP_VERTICAL 2
#define GD_FLIP_BOTH 3

/**
* Group: Crop
*
Expand Down

18 comments on commit bdc281e

@remicollet
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this totally break the build of PHP

@remicollet
Copy link
Contributor

Choose a reason for hiding this comment

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

@pierrejoye can you please revert ? and push a 2.3.4 ?

@pierrejoye
Copy link
Contributor Author

@pierrejoye pierrejoye commented on bdc281e Sep 13, 2021 via email •
edited
Loading

Choose a reason for hiding this comment

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

@remicollet
Copy link
Contributor

Choose a reason for hiding this comment

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

They are used in PHP

https://github.com/php/php-src/blob/master/ext/gd/gd.c#L403

So removing them break the build of PHP

@pierrejoye
Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me check the code there. I don't remember, maybe it is just used to select the right function. In this case I am more to define them there instead of re introducing them here, as they are not used anywhere. What do you think?

Aslo, btw, I pinged you in a few tickets, any chance to look at them? maybe better to catch such things in the CI before :)

@remicollet
Copy link
Contributor

@remicollet remicollet commented on bdc281e Sep 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

let me check the code there. I don't remember, maybe it is just used to select the right function. In this case I am more to define them there instead of re introducing them here, as they are not used anywhere. What do you think?

Yes defined for user land and used in https://github.com/php/php-src/blob/master/ext/gd/gd.c#L3601

Yes, we should defined them in PHP, not in libgd, BUT this is only for future version of PHP, and probably only for 8.1+
So still have to be re-added (probably with a huge comment) for old versions (7.3 to 8.0)

Aslo, btw, I pinged you in a few tickets, any chance to look at them? maybe better to catch such things in the CI before :)

Quite busy, but will do

@pierrejoye
Copy link
Contributor Author

@pierrejoye pierrejoye commented on bdc281e Sep 13, 2021 via email

Choose a reason for hiding this comment

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

@remicollet
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ok to add an option to include them and clearly document the why?

I don't think it worth the work...
A simple comment should be enough, as already exists for the typo

@remicollet
Copy link
Contributor

Choose a reason for hiding this comment

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

@pierrejoye another issue (missing gd_io_stream.h)... looking at it right now

@pierrejoye
Copy link
Contributor Author

@pierrejoye pierrejoye commented on bdc281e Sep 13, 2021 via email

Choose a reason for hiding this comment

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

@pierrejoye
Copy link
Contributor Author

@pierrejoye pierrejoye commented on bdc281e Sep 13, 2021 via email

Choose a reason for hiding this comment

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

@remicollet
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, but how older php can be built with 2.3.4 then?

If you add the macro back.... It will work.... or don't understand your question

how can that happen, nothing was changed for that... Damned. Only with PHP?

No, running abi-compliance checker.
It is referenced in gdpp.h

@pierrejoye
Copy link
Contributor Author

@pierrejoye pierrejoye commented on bdc281e Sep 13, 2021 via email

Choose a reason for hiding this comment

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

@remicollet
Copy link
Contributor

Choose a reason for hiding this comment

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

See https://rpms.remirepo.net/compat_reports/libgd/2.3.2_to_2.3.3/compat_report.html

I use https://github.com/lvc/abi-compliance-checker

@remicollet
Copy link
Contributor

Choose a reason for hiding this comment

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

Notice: the removal of entities.h is detected as a major issue (symbol removal), btw, should not be used (internal stuff), and in all case the name is terrible for something public.

@pierrejoye
Copy link
Contributor Author

@pierrejoye pierrejoye commented on bdc281e Sep 13, 2021 via email

Choose a reason for hiding this comment

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

@remicollet
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, see php/php-src#7490

@oerdnj
Copy link
Contributor

@oerdnj oerdnj commented on bdc281e Jul 11, 2022

Choose a reason for hiding this comment

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

This change breaks the PHP build for all PHP versions from 5.6 to 8.0 (I haven't checked the PHP 5.5 and below).

Personally, I am going to revert the change in libgd Debian package, it's less hassle than patching all the PHP versions still packaged by my repository.

Please sign in to comment.