Skip to content

[4.0] Adding version switch in Image and Imagefilter for PHP8#28694

Merged
HLeithner merged 6 commits intojoomla:4.0-devfrom
Hackwar:j4php8
May 29, 2020
Merged

[4.0] Adding version switch in Image and Imagefilter for PHP8#28694
HLeithner merged 6 commits intojoomla:4.0-devfrom
Hackwar:j4php8

Conversation

@Hackwar
Copy link
Copy Markdown
Member

@Hackwar Hackwar commented Apr 16, 2020

PHP8 changed the behavior of GD to use objects instead of resources. This introduces version dependent handling in the code and thus should make this work again. Since we have good unittests for this, this should be enough to test this.

@richard67
Copy link
Copy Markdown
Member

richard67 commented Apr 16, 2020

@Hackwar Just a question from reading: Ist it correct that the first check if $source is true applies only to the first || condition in lines 128 and 129?

if ($source && (\is_object($source) && get_class($source) == 'GdImage')
	|| (\is_resource($source) && get_resource_type($source) == 'gd'))

Or should it apply in both of the cases separated by the || as follows:

if ($source && (\is_object($source) && get_class($source) == 'GdImage'
	|| \is_resource($source) && get_resource_type($source) == 'gd'))

As I said, just a question from reading, maybe all is fine as is it.

@Hackwar
Copy link
Copy Markdown
Member Author

Hackwar commented Apr 16, 2020

No, that is fine the way it is. $source can be null and get_class(null) (or anything not an object) will throw an error.

Anyhow, with this PR and the bunch of changes we did to drone and the docker images used, I can now happily proclaim that we are passing all tests.

@zero-24
Copy link
Copy Markdown
Contributor

zero-24 commented Apr 16, 2020

hmm can you think of any reason that RIPS would complain about files in this path: libraries/vendor/hoa I don't see that this PR touches anything around that path and is also not introducing new composer deps as well as this path does not seem to be included in with an composer install?

@SniperSister
Copy link
Copy Markdown
Contributor

@zero-24 seems to be a RIPS glitch. Whitelisted the issues and restarted the build

@HLeithner
Copy link
Copy Markdown
Member

We should be consistent on how we handle this. I think its enough if we check for object and resource (including type) and not for the php version. Also please add a comment that resource can be removed if we only support php 8+

Comment on lines -130 to +134
$this->handle = &$source;
$this->handle = $source;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure if this by-reference has any effect. In the end we are only handing over a pointer here, right? So it shouldn't matter if we hand this over by reference or not. Can anybody confirm me on that?

@Hackwar
Copy link
Copy Markdown
Member Author

Hackwar commented Apr 17, 2020

I added the comments and unified the handling. Please review again and give thumbs up/down.

@HLeithner HLeithner merged commit 65049d1 into joomla:4.0-dev May 29, 2020
@HLeithner
Copy link
Copy Markdown
Member

Thanks

@HLeithner HLeithner added this to the Joomla 4.0 milestone May 29, 2020
sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020
…#28694)

* Adding version switch in Image and Imagefilter for PHP8

* Simplifying the logic

* Removing logic inversion

* Unifying handling of feature check
@Hackwar Hackwar deleted the j4php8 branch October 23, 2020 20:16
brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request Sep 21, 2023
As we are now using php8 the TODO has been resolved and the code can be removed

See comment joomla#28694 (comment)
LadySolveig pushed a commit that referenced this pull request Dec 6, 2023
PHP8 changed the behavior of GD to use objects instead of resources.
As we are now using php8 the TODO has been resolved and the code can be removed
See comment #28694 (comment)
brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request Oct 16, 2024
Resolve todo as we only support php8 now.

This should probably have been done when I did joomla#41855

Original PR where this check was added joomla#28694

Signed-off-by: BrianTeeman <brian@teeman.net>
brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request Oct 16, 2024
Resolve todo as we only support php8 now.

This should probably have been done when I did joomla#41855

Original PR where this check was added joomla#28694

Signed-off-by: BrianTeeman <brian@teeman.net>
@brianteeman brianteeman mentioned this pull request Oct 16, 2024
4 tasks
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

Successfully merging this pull request may close these issues.

6 participants