Skip to content

Refactor HashContext into an object. #660

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

Closed
wants to merge 1 commit into from

Conversation

realityking
Copy link
Contributor

This is essentially #611 without adding methods to the object.

Tests all pass, one required modification as it relied on the output of var_dump. The B/C break is very small, code that relies on checking is_resource() fails and needs to be changed to also do ìnstanceof HashContext`.

The new class offers only __construct() and a clone handler, everything else has to be done to he procedural API.

HashContext can also be serialized. Currently this is simply binary data. I wonder if this should be base64/base85 encoded?

@lstrojny
Copy link
Contributor

Base64-encoding makes sense

@realityking
Copy link
Contributor Author

Sorry, I was with my head in a different branch. This class does not support serialize. (Though I will use base64 for that branch.)

@ghost
Copy link

ghost commented Mar 7, 2015

Can one of the admins verify this patch?

@sgolemon
Copy link
Member

I'll pick this up to update it for PHP7 and propose an RFC for it. Will keep your name on the commit of course.

@sgolemon
Copy link
Member

I've rewritten this patch for PHP7 as master...sgolemon:pr-660 with a couple notable differences:

  1. Rather than dtor the by-value object passed into hash_final (which won't quite do what you think it'll do), I've used hash->context as a marker of the object's "validity". Once the hash is finalized, context gets freed, and any further attempts to use the object will throw an error matching the previous "invalid resource" message (see attached test).
  2. I didn't add the method API. I in fact marked the constructor as private. This is just to keep the migration focused on the type change. We can (and should) follow it up with another diff to add methods.

@krakjoe
Copy link
Member

krakjoe commented Dec 27, 2016

ping @sara, there are conflicts, I didn't look close, sorry if silly ...

@sgolemon
Copy link
Member

@krokjoe The conflicts are on @realityking's original PR (this diff), which is against PHP5. My rewrite of it is over at master...sgolemon:pr-660

@php-pulls
Copy link

Comment on behalf of pollita at php.net:

To minimize confusion, I'm closing this PR. Please reference my branch (linked above) for the current patch.

@php-pulls php-pulls closed this Dec 27, 2016
@sgolemon
Copy link
Member

Also... @krakjoe, not @krokjoe :p

@nikic
Copy link
Member

nikic commented Dec 27, 2016

@sgolemon Can you please also open a PR for your branch? Otherwise not possible to review.

@sgolemon
Copy link
Member

@nikic Here ya go: #2309

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

Successfully merging this pull request may close these issues.

7 participants