bluehousegroup / silverstripe-pardot

BSD 3-Clause "New" or "Revised" License
4 stars 5 forks source link

NEW: Change encryption to work with PHP 7.1+ #24

Closed sminnee closed 5 years ago

sminnee commented 5 years ago

This also requires some different set-up procedures as detailed in the README change.

It would be good to release this as 1.1.0.

Fixes #23

NightJar commented 5 years ago

So the basis of this PR is that mcrypt is deprecated in PHP7, then removed in PHP7.3, so you've updated to use the Defuse cryptography library to perform the same functions? If so, ideally the commit message could reflect this :)

Do you think these changes could be done without altering the API? Or do you think the method signature alterations around pardot_encrypt and pardot_decrypt are safe?

In the very least protected function getPardotPassword() has been removed, which could be breaking for any subclass relying on it (unlikely, but semver is what it is). Although this function seems superfluous, I don't think removing it achieves anything, does it?

sminnee commented 5 years ago

The issue is that there’s no room to do a major release as Vn+1 is the

I can add the method back, though, and update the commit message

NightJar commented 5 years ago

I realise that. I was looking more for confirmation of thought though sorry. PHP doesn't care if we pass a method more parameters than are necessary... however how does this affect subclasses? Will they suddenly start failing with notices about signatures not matching?

I was curious, so I tested.

<?php

class TheParent
{
    public function someNoise($thing1)
    {
        $thing2 = 2;
        return "$thing1 $thing2";
    }
}

class TheChild extends TheParent
{
    public function someNoise($first, $second = null)
    {
        return parent::someNoise($first, $second);
    }
}

var_dump(
    (new TheParent)->someNoise('one'),
    (new TheParent)->someNoise('one', 'two'),
    (new TheChild)->someNoise('one'),
    (new TheChild)->someNoise('one', 'two')
);

No warning or other kind of error resulted. Thus, I think the signature changes are actually safe (regarding the necessity of a minor release).

Restoring the removed function and updating the commit comment would see me happy.

Not that I can merge or anything ;p

sminnee commented 5 years ago

Cool that's done now. Look good now, @NightJar ?

NightJar commented 5 years ago

Neato, thanks @sminnee! :)