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_SSH2_class] Introduce a fix for inconsistent return type problem." #1472

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

Conversation

func0der
Copy link

@func0der func0der commented Apr 21, 2020

Proposed fix for #1471

I just started replacing all return false with exceptions. This is just a proposition. Before I migrate the whole class, I would like some input on this :)

There might be some problems with other classes, since there were private classes used outside of the scope of the class SSH2 (for example in SCP), but they might be resolvable by simply introducing real public methods that wrap the private ones, catch the new exceptions and return the old (read: wrong) return types.

@func0der func0der changed the base branch from master to 2.0 April 21, 2020 14:51
@terrafrost
Copy link
Member

Throwing exceptions in the 2.0 branch is a non starter as it's a BC break. The 3.0 branch, however, does throw exceptions.

As for the issue that this is attempting to fix... I'll take a look in the next few days as time permits!

Thanks!

@func0der
Copy link
Author

Throwing exceptions in the 2.0 branch is a non starter as it's a BC break.

The only changes made are those to "private" methods (those with an _ (underscore) in front of it). If someone was to use those methods, they would use the library wrong.
Also the @access PHPDoc states that those methods are private.

It would be possible to have the internal code throw exceptions and the external api to keep returning the falsely documented results like "false" etc.

@terrafrost
Copy link
Member

It would be possible to have the internal code throw exceptions and the external api to keep returning the falsely documented results like "false" etc.

The public functions would need to catch the exceptions thrown by the private exceptions and return false, if appropriate, instead. At least for the code changes not to be BC breaking.

Like consider this example:

function a()
{
    throw new \Exception('demo');
}

function b()
{
    a();
}

b();

The exception a() throws will kinda float up to b().

And make no mistake the changes are BC breaking. Exceptions do offer a lot of advantages over doing user_error() but user_error() won't kill a program whereas an uncaught exception will.

Also, I'd do one or the other (user_error() or throwing an exception) - not both.

Finally, I'm not the biggest fan of including the full SSH packet in the exceptions. The full SSH packet is going to be binary encoded and ran over a command line it could mess up how stuff displays. As a simple demonstration...

for ($i = 128; $i <= 255; $i++) {
    echo chr($i);
}

Does that actually output anything for you? It doesn't output anything for me.

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