Skip to content

Conversation

marcosptf
Copy link
Contributor

a new test to show whats openssl md_methods are avaliables

a new test to show whats openssl md_methods are avaliables
@bukka
Copy link
Member

bukka commented Oct 25, 2015

The same as cipher algs test. It is incorrect as the list is dependent on installation...

@marcosptf
Copy link
Contributor Author

Hi @bukka !
everything fine?

thanks for your review, i've fixed my mistake;
:-)

Copy link
Member

Choose a reason for hiding this comment

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

please remove this

@jerrygrey
Copy link

Couldn't this be more simpler?

...
--FILE--
<?php
var_dump(is_array(openssl_get_md_methods(true)));
var_dump(is_array(openssl_get_md_methods(false)));
?>
--EXPECT--
bool(true)
bool(true)

Also, would using empty() be better? Would it be bad if this method return an empty array?

@marcosptf
Copy link
Contributor Author

Hi @jerrygrey how are you?
fine?

you are right, this way it's more simple, but i like show a message explain what was wrong if the test function fail.

Thanks for your comment!

:-)

@jerrygrey
Copy link

@marcosptf The test script will automatically provide an error message on failure, along with dffs, etc to make it easy to find out what went wrong, so those types of error messages are not really needed tbh.

Copy link
Member

Choose a reason for hiding this comment

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

tab instead of spaces pls

@marcosptf
Copy link
Contributor Author

@bukka @jerrygrey
the changes is done

@jerrygrey
Copy link

@marcosptf Looks good, I don't see any issues with the code 👍

@marcosptf
Copy link
Contributor Author

@bukka @jerrygrey
thanks for your support
👍

@marcioAlmada
Copy link
Contributor

The history looks very confusing. How about squashing the commits here and also on #1601?

😉

@marcosptf
Copy link
Contributor Author

@bukka @jerrygrey
done!

@bukka
Copy link
Member

bukka commented Oct 27, 2015

@marcosptf
It looks good. Could you open a new PR with just one commit containing this and #1601 so I can cherry pick it and you will stay as an author... ;)

I think there are actually some md algs that is not possible compile OpenSSL without (e.g. md5 - even no-md5 options is there - think it fails - I'll give a try) so I will add them later on top of your commit.

@marcosptf
Copy link
Contributor Author

Hi @bukka

let me see if i understand.
i need create a new PR, with this test "array openssl_get_md_methods ([ bool $aliases = false ] );" plus this test "array openssl_get_cipher_methods ([ bool $aliases = false ] );" ???

but, is correct write a test file to two functions?

explain to me please

Thanks for your patience!

:-)

@jerrygrey
Copy link

@marcosptf No, keep the two tests in two different test files, @bukka just wants them both on one PR with one commit, it is just to keep everything nice and tidy on here.

@bukka
Copy link
Member

bukka commented Oct 29, 2015

@marcosptf Exactly as @jerrygrey says . One PR with one commit containing two files.

@marcosptf
Copy link
Contributor Author

@bukka @jerrygrey
like this ==> #1612 ???

:D

@jerrygrey
Copy link

@marcosptf 👍

@marcosptf
Copy link
Contributor Author

Hi @bukka @jerrygrey,
i would like to know, if you can see another PR?
to see if i did something wrong or it's correct
is this PR ===> #1553

i'm waiting for your comments
thanks so much!

:-)

@php-pulls
Copy link

Comment on behalf of bukka at php.net:

Part of PR #1612 - closing

@php-pulls php-pulls closed this Oct 29, 2015
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.

5 participants