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

Getting an error when listing experiments - unknown (new) attributes such as 'selector' causing exceptions to be thrown #1

Closed
LCD344 opened this issue May 4, 2017 · 12 comments
Assignees
Labels

Comments

@LCD344
Copy link

LCD344 commented May 4, 2017

Hi!

I have optimizely web account and I'm getting an error with

Fatal error: Uncaught WebMarketingROI\OptimizelyPHP\Exception: Unknown option: selector in C:\wamp64\www\optimizely\vendor\webmarketingroi\optimizely-php\lib\WebMarketingROI\OptimizelyPHP\Resource\v2\Change.php on line 106

When I try to run the most simple experiments query:

$client = new OptimizelyApiClient([
	'access_token' => getenv('OPTIMIZELY_TOKEN')
], 'v2');


$result = $client->experiments()->listAll(getenv('PROJECT_ID'));

thanks!

@jamesspittal
Copy link
Member

Hi @LCD344

Thanks for the ticket/issue. It looks like (from your message above) that you're doing this via WAMP (as opposed to a *nix environment). I must admit we haven't tested this thoroughly via WAMP. Are you able to share what version of PHP you're running via WAMP?

@jamesspittal
Copy link
Member

Hi @LCD344

Doing some further investigation into this, we've been able to reproduce this same error message at our end. Your query example (listing all experiments) used to work previously. It looks like the Optimizely engineering team working on the REST API might have added a new attribute ('selector') - which didn't exist previously.

Essentially, this case is not currently catered for in the Default Constructor within Change.php here:

    public function __construct($options = array())
    {
        foreach ($options as $name=>$value) {
            switch ($name) {                
                case 'type': $this->setType($value); break;
                case 'allow_additional_redirect': $this->setAllowAdditionalRedirect($value); break;
                case 'async': $this->setAsync($value); break;
                case 'css_selector': $this->setCssSelector($value); break;
                case 'dependencies': $this->setDependencies($value); break;
                case 'destination': $this->setDestination($value); break;
                case 'extension_id': $this->setExtensionId($value); break;
                case 'preserve_parameters': $this->setPreserveParameters($value); break;
                case 'src': $this->setSrc($value); break;
                case 'value': $this->setValue($value); break;
                case 'id': $this->setId($value); break;
                default:
                    throw new Exception('Unknown option: ' . $name);
            }
        }
    }

We'll do some further experimentation and see if a patch (to extend support for this new attribute) is the best course of action here (cc @olegkrivtsov). Apologies for the inconvenience.

@jamesspittal
Copy link
Member

I also noticed this line:

                case 'css_selector': $this->setCssSelector($value); break;

To me, 'selector' and 'css_selector' seem like the same thing. Possibly, that may have been renamed in the REST API. More investigation required.

@LCD344
Copy link
Author

LCD344 commented May 4, 2017

@jamesspittal

Thanks for the swift response, I'm actually toying around with the code (As I've hit various errors). I commented out the exception throwing and this makes things works (but the exceptions were thrown also in Change.php and Metrics.php - in all cases it's the same error, exception thrown for an unknown field)

Another thing that happened is that I can't update an experiment:

(Details/issue logged in #1 by @jamesspittal)

If I manage to advance with this I will report back

@jamesspittal
Copy link
Member

jamesspittal commented May 5, 2017

Hi @LCD344

Thanks for toying with the code - glad to hear you were able to get things working by disabling the exception throwing (although that may not be a good idea - ideally, exception throwing for unrecognised attributes is good as long as the underlying API doesn't change/break backwards compatibility).

Pull requests are always happily accepted. If you could also submit separate issues for each of the problems you've discovered - that would be great.

We're also working on a patch to appropriately handle the latest updates to the REST API right now (new attributes/attribute names). I'll keep you updated on that front.

@jamesspittal
Copy link
Member

Hi @LCD344 - I've logged your other issue into #2

@jamesspittal jamesspittal changed the title Getting an error when listing experiments Getting an error when listing experiments - unknown (new) attributes such as 'selector' causing exceptions to be thrown May 5, 2017
@LCD344
Copy link
Author

LCD344 commented May 5, 2017

@jamesspittal - thanks, I'm a bit busy with traveling, but once I'm done, and back to working on that thing I will organize everything into proper bug reports and post pull requests, if where needed - maybe you should have a non breaking mode for new patches (a flag to say not throw exceptions on unknown properties?)

@jamesspittal
Copy link
Member

Thanks @LCD344

You mentioned:

maybe you should have a non breaking mode for new patches (a flag to say not throw exceptions on unknown properties?)

This is an interesting suggestion, we'll definitely consider that (although the advantage of the exception throwing on unknown properties is a good default option in my opinion, kind of like strict mode - especially given that the v2 REST API is still evolving/changing quickly with the Optimizely engineering team at this point in time).

The good news is that we should have a new release/commit with these bugs fixed available shortly. @olegkrivtsov will update this as soon as that's ready for usage at your end (and you should be able to pull it down via composer easily).

olegkrivtsov added a commit that referenced this issue May 8, 2017
…ributes such as 'selector' causing exceptions to be thrown #1
@olegkrivtsov
Copy link
Contributor

olegkrivtsov commented May 8, 2017

Hi @jamesspittal, I've tried to fix this, but some integration tests still don't work, for example for experiments and attributes. I've asked Optimizely support for help with those problems. Still, with this commit @LCD344 should have some working code (and should be able to do what they're hoping to do).

@jamesspittal
Copy link
Member

Thanks @olegkrivtsov

@LCD344 Could you grab the latest release from packagist (0.9.0beta2) - https://packagist.org/packages/webmarketingroi/optimizely-php and let us know if you're still having any problems listing experiments?

@LCD344
Copy link
Author

LCD344 commented May 8, 2017

It works! thanks!

@jamesspittal
Copy link
Member

It works! thanks!

Great - thanks for the feedback @LCD344 - closing this issue now.

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

No branches or pull requests

3 participants