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

Imgopt reorg with tweaks #197

merged 22 commits into from Oct 7, 2018


None yet
2 participants
Copy link

zytzagoo commented Oct 4, 2018

Fixes #194 among other things.

Would be great to get this merged before any other imgopt-related changes/fixes start happening.

zytzagoo added some commits Sep 13, 2018

Revert "bump composer deps"
This reverts commit 2e29ce7.
Not a fan of is_null() (might be just a habit thing?)
If we go the is_null() route, we should use \is_null() probably?
some readability changes
all those underscores make my brain hurt
normalize_img_urls -> normalize_img_url + caching results
The rename is due to the method not really working with multiple input urls
(and it's working on a string, not array) but rather a single one.

The caching is there since it's called multiple times with identical inputs.
full test coverage of autoptimizeImages::normalize_img_url()
also takes care of an edge case when a root-relative url is given to can_optimize_image(), which checks the hostname, which is non-existent in root-relative urls, and that throws a warning/notice...

@zytzagoo zytzagoo requested a review from futtta Oct 4, 2018


This comment has been minimized.

Copy link

futtta commented Oct 4, 2018


This comment has been minimized.

Copy link
Collaborator Author

zytzagoo commented Oct 5, 2018

It shouldn't change functionally, apart from some improvements/fixes mentioned in the commits above -- and those tests that were in the suite still pass.

It incorporates changes/fixes done on beta so far (with regards to imgopt stuff).

How else can we test this?

I think it should be safe to go in the next minor release really, but I understand the fear :)

Can we merge in beta branch and bump the beta version so it gets out to some of beta users before releasing as 2.4.1?


This comment has been minimized.

Copy link

futtta commented Oct 7, 2018

I think we need to do this differently after all @zytzagoo, I'd like to propose;

  • we release 2.4.1 in approx. this shape, so without your imgopt changes (which I consider valuable!)
  • we update master to that version
  • we then merge your changes in beta and change the version into 2.5.0-alpha (allowing beta-testers to update)
  • bugfixes & small improvements go to both master & beta
  • bigger changes only go into beta
  • we change the version string to 2.5.0-beta when we think we're getting close to being feature-complete (or there is a least one major functional improvement in there)

Goal is simply to keep "minor" releases really minor, only addressing real-life bugs (or small improvements) with a minimum of code changes with the simple goal to limit the risk or regressions.

Maybe I'm just being too careful, but I'd rather be too careful and not drown in support questions of which there have been a lot after 2.4.0 ;-)

@futtta futtta merged commit a5c6b50 into futtta:beta Oct 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed

@zytzagoo zytzagoo deleted the zytzagoo:imgopt-review branch Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.