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

fix: remove deprecated domain sharding functionality #45

Merged
merged 5 commits into from
Jun 11, 2019

Conversation

sherwinski
Copy link
Contributor

This PR removes all domain sharding functionality from imgix-php, following its deprecation in #42

@sherwinski sherwinski requested a review from jayeb June 10, 2019 22:45
Copy link
Contributor

@jayeb jayeb left a comment

Choose a reason for hiding this comment

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

One dumb little request, but otherwise this looks great.

@trigger_error($warning_message, E_USER_DEPRECATED);
$this->domains = $domains;
if (!is_string($domain)) {
throw new \InvalidArgumentException("UrlBuilder must be passed a valid string domain");
Copy link
Contributor

Choose a reason for hiding this comment

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

pushes up glasses

Can we change this message to "UrlBuilder must be passed a string domain"? The validity of the string isn't technically relevant here, since validity isn't checked until L19.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that to be consistent with imgix-core-js but I see your point. If we change it here, should we carry it over everyone else that applies?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah dinger, shoulda caught it there. I'd say change it here and make a note somewhere to roll that in the next time we change something in imgix-core-js. I don't think it's worth downshifting just to go back and change that, I'd rather you keep forging ahead.

@jayeb
Copy link
Contributor

jayeb commented Jun 11, 2019

Oh, and obviously these broken tests are a problem.

@sherwinski
Copy link
Contributor Author

Sigh yea, I'm combing through phpunit's github & travis documentation to see if there's a quick fix to this. I'm not sure how travis determines what version of phpunit should be used per job, but I don't think it should be defaulting to the version that isn't supported on a particular version of PHP. Ideally I'd like to avoid just having to remove those versions altogether.

@sherwinski sherwinski merged commit 7212d3a into master Jun 11, 2019
@sherwinski sherwinski deleted the remove-domain-sharding branch June 11, 2019 19:09
sherwinski pushed a commit that referenced this pull request Dec 18, 2020
Introduced in #45, this pre-flight script ensured a compatible version of phpunit was installed for php versions ≥ 7.0
sherwinski pushed a commit that referenced this pull request Dec 18, 2020
Introduced in #45, this pre-flight script ensured a compatible version of phpunit was installed for php versions ≥ 7.0
sherwinski pushed a commit that referenced this pull request Dec 23, 2020
Introduced in #45, this pre-flight script ensured a compatible version of phpunit was installed for php versions ≥ 7.0
sherwinski pushed a commit that referenced this pull request Jan 4, 2021
Introduced in #45, this pre-flight script ensured a compatible version of phpunit was installed for php versions ≥ 7.0
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.

None yet

2 participants