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

[8.0] Update EasyHandle in order to avoid bad writes of properties related to the handle #2293

Open
wants to merge 1 commit into
base: 8.0
Choose a base branch
from

Conversation

phansys
Copy link
Contributor

@phansys phansys commented Apr 27, 2019

Q A
Branch master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

With these changes, setting fake values like this can be prevented.

@phansys phansys force-pushed the easy_handle branch 3 times, most recently from 9ff0a64 to a1d7847 Compare April 29, 2019 14:52
@Nyholm
Copy link
Member

Nyholm commented Oct 18, 2019

Thank you for the PR.
I would like you to clarify one thing for me: From whom are you preventing to set fake values?

@phansys
Copy link
Contributor Author

phansys commented Oct 18, 2019

The prevention is done in order to avoid the object to accept a set of inconsistent values, regardless who is writing them. In my specific use case, the situation was caused from a 3rd party library using Guzzle, but I don't have enough details since I think the problem was related to #2292.

@Nyholm Nyholm added this to the 8.0.0 milestone Dec 8, 2019
@phansys phansys force-pushed the easy_handle branch 2 times, most recently from 0975ccd to c782faf Compare January 6, 2020 17:53
@@ -147,7 +147,7 @@ private static function finishError(

// Retry when nothing is present or when curl failed to rewind.
if (empty($easy->options['_err_message'])
&& (!$easy->errno || $easy->errno == 65)
&& (!$easy->errno || $easy->errno == /* CURLE_SEND_FAIL_REWIND */ 65)
Copy link
Member

Choose a reason for hiding this comment

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

When is this constant not set? Was added at least 10 years ago, wasn't it?

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 don't think so. See php/php-src#4079.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. They never made it into PHP-land. ;)

@phansys phansys force-pushed the easy_handle branch 2 times, most recently from 7f8c70a to 0998eab Compare January 6, 2020 22:09
@stale
Copy link

stale bot commented Sep 25, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 2 weeks if no further activity occurs. Thank you for your contributions.

@stale stale bot added the lifecycle/stale No activity for a long time label Sep 25, 2020
@Nyholm Nyholm added the lifecycle/keep-open Issues that should not be closed label Sep 25, 2020
@stale stale bot removed the lifecycle/stale No activity for a long time label Sep 25, 2020
@phansys phansys force-pushed the easy_handle branch 4 times, most recently from 8e43093 to dd0ea36 Compare June 9, 2021 00:25
src/Handler/EasyHandle.php Outdated Show resolved Hide resolved
@phansys phansys force-pushed the easy_handle branch 6 times, most recently from 2a25470 to 36c07f3 Compare June 11, 2021 00:06
@GrahamCampbell GrahamCampbell removed this from the 8.0.0 milestone Mar 31, 2024
@GrahamCampbell GrahamCampbell changed the title Update EasyHandle in order to avoid bad writes of properties related to the handle [8.0] Update EasyHandle in order to avoid bad writes of properties related to the handle Mar 31, 2024
@GrahamCampbell GrahamCampbell changed the base branch from 7.5 to 8.0 March 31, 2024 19:45
@GrahamCampbell
Copy link
Member

@phansys are you able to resolve the merge conflicts please?

@phansys phansys force-pushed the easy_handle branch 6 times, most recently from c14b88a to 229317f Compare April 1, 2024 12:41
@phansys
Copy link
Contributor Author

phansys commented Apr 1, 2024

@phansys are you able to resolve the merge conflicts please?

Since this PR is almost 5 years old, I guess I must also update the behavior on some checks, since the added support for PHP >= 8 in the target branch also changed some return values in CURL functions (like resource => CurlHandle).

@phansys phansys force-pushed the easy_handle branch 2 times, most recently from ed9dfc1 to 2245ac1 Compare April 1, 2024 13:48
}

/**
* @throws \LogicException
Copy link

Choose a reason for hiding this comment

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

Where does it throw LogicException in __unset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This exception isn't thrown in the current implementation. The docblock was updated, thank you.

@phansys phansys force-pushed the easy_handle branch 2 times, most recently from 267b28b to 0e8c2ca Compare April 1, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/keep-open Issues that should not be closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants