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

Add webp support to Image Manipulation Class #3084

Merged
merged 2 commits into from
Jul 1, 2020

Conversation

nicojmb
Copy link

@nicojmb nicojmb commented Jun 10, 2020

Each pull request should address a single issue and have a meaningful title.

Description
Support webp image in Image Manipulation Class

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • [x ] Conforms to style guide

---------Remove from here down in your description----------

Notes

  • Pull requests must be in English
  • If the PR solves an issue, reference it with a suitable verb and the issue number
    (e.g. fixes 12345)
  • Unsolicited pull requests will be considered, but there is no guarantee of acceptance
  • Pull requests should be from a feature branch in the contributor's fork of the repository
    to the develop branch of the project repository

@lonnieezell
Copy link
Member

It is possible for Imagemagick to support webp if it's compiled in so please make sure the Imagemagick handler also works with it or throws the error if not.

system/Images/Image.php Outdated Show resolved Hide resolved
@michalsn
Copy link
Member

@nicojmb we have two things to fix here:

  • The first - tests. If you look at Travis, we have 3 tests that are failing now, we have to change constant from IMAGETYPE_PNG to IMG_PNG since this is the correct one returned by getimagesize() function.
  • The second thing that I didn't notice earlier is that your commits are not signed and we can't merge your code. Please check this page to learn more about code signing: https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/signing.rst

@MGatner is there a way to sign the code right now, to make it mergeable? I think you showed me once some instructions but I didn't succeed make them work for me (it was probably my fault). So, I'm just wondering is there a way to "fix" this PR in this state.

@MGatner
Copy link
Member

MGatner commented Jun 12, 2020

@michalsn you can add credentials to GitHub post-push and it will verify previous commits, but if the commits weren’t signed already that won’t work. Anyone could pull the fork, git reset develop, then re-add and re-commit a signed version and make a new PR. Or Lonnie could just override the verification requirement 😇

@MichalPB1
Copy link

Can it be merged? This is something that is useful to me :)

@michalsn
Copy link
Member

Right now we can't merge it since it's not signed and tests are failing. If nothing will change, I will probably wait till the weekend and try to fork this branch and re-commit.

@lonnieezell
Copy link
Member

Yeah, the biggest problem is the tests. I am happy to override. And am thinking about removing that requirement, anyway, actually.

@michalsn
Copy link
Member

Yes, removing the code signing requirement would probably be a good idea.

@michalsn michalsn mentioned this pull request Jun 27, 2020
5 tasks
@lonnieezell lonnieezell merged commit eaf9f02 into codeigniter4:develop Jul 1, 2020
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.

5 participants