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

Rollback changes from PR #657 #839

Merged
merged 1 commit into from
Nov 8, 2023
Merged

Conversation

laszlof
Copy link
Contributor

@laszlof laszlof commented Nov 8, 2023

This change rolls back a bug fix from PR #657. This PR contained no test's, and no real documentation what the issue was it was trying to solve. As a result, this change has broken updates (amongst other things) on at least anyone using curl 7.29.

See Issue #838 for more details.

Pull Request Type

  • I have checked there is no other PR open for the same change.

This is a:

  • Bug fix
  • New feature
  • Documentation improvement
  • Code quality improvement

Context

Roll back a bug fix from PR #657.

Detailed Description

Quality assurance

  • This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added unit tests to accompany this PR.
  • The (new/existing) tests cover this PR 100%.
  • I have (manually) tested this code to the best of my abilities.
  • My code follows the style guidelines of this project.

Documentation

For new features:

  • I have added a code example showing how to use this feature to the examples directory.
  • I have added documentation about this feature to the docs directory.
    If the documentation is in a new markdown file, I have added a link to this new file to the Docs folder README.md file.

@schlessera schlessera merged commit d476725 into WordPress:develop Nov 8, 2023
19 checks passed
@schlessera schlessera mentioned this pull request Nov 8, 2023
5 tasks
@jrfnl
Copy link
Member

jrfnl commented Nov 8, 2023

For the record: the underlying issue #656 contains information on what #657 solves.

schlessera added a commit that referenced this pull request Nov 8, 2023
@@ -383,7 +382,7 @@ private function setup_handle($url, $headers, $data, $options) {
$options['hooks']->dispatch('curl.before_request', [&$this->handle]);

// Force closing the connection for old versions of cURL (<7.22).
Copy link

Choose a reason for hiding this comment

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

This comment is no longer valid.

Copy link
Member

Choose a reason for hiding this comment

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

The comment was already present before we made the problematic change and was the reason why we used the specific version constraint we ended up with. Before we fully understand the context and where this was originally coming for, I'll prefer to keep in in there.

Choose a reason for hiding this comment

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

The version number in the comment could be a cause for confusion going forward, just a heads up.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, indeed, and I think that's what has already happened in this case.
But I'm still assuming there is a reason why this was put in there initially. I'd like to understand this better before removing it.

Copy link
Member

Choose a reason for hiding this comment

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

For future reference, this is the PR that introduced the comment: #211

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants