Skip to content

Let's see if we can pull the latest changes#6

Open
jakubsuchy wants to merge 31 commits into
jakubsuchy:masterfrom
froodley:master
Open

Let's see if we can pull the latest changes#6
jakubsuchy wants to merge 31 commits into
jakubsuchy:masterfrom
froodley:master

Conversation

@jakubsuchy

Copy link
Copy Markdown
Owner

No description provided.

Pete Burkindine added 30 commits February 10, 2017 15:58
…rs for child classes. Added RequestInterface to improve testability/strong typing. PSR.
…ted; just returning instead of throwing if [intent][slots] is missing
@jakubsuchy

Copy link
Copy Markdown
Owner Author

For one, I am unsure if Purifier is needed. All the data from Amazon is very strict - it's either a predefined value or a predefined format we know about. We should be able to avoid a library and purify by checking for explicit data types...

Certificate will always be a URL
Slot name is always a string with no special characters
Application ID is always a string with [a-z][0-9] and - characters only...

@chris-hamper chris-hamper left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These changes give the library a much cleaner/more polished feel, and add some valuable features, like card types that include images. Overall the benefits of pulling in these changes would seem to outweigh the costs. It would also be great to reduce fragmentation between the different forks, so that future contributions could also be used.

I'll do a bit of testing to see what impact this has if used by the Drupal module.

Comment thread README.md
## Usage

Install via composer: `composer require minicodemonkey/amazon-alexa-php`.
Install via composer: `composer require froodley/amazon-alexa-php`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Use the correct repo name here


// Fields

public static $validTypes = [

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't like this tight coupling on valid request types. It seems like this will break if Amazon adds a new request.

// Traits

namespace Alexa\Request;
use HasPurifier;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree, using Purifier seems like overkill, but I suppose it could help protect from some theoretical XSS/spoofing attacks.

Comment thread composer.json
},
"require-dev": {
"phpunit/phpunit": "~4.6"
"phpunit/phpunit": "^5.7.15"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This will conflict with Drupal's current dependency on ">=4.8.35 <5". Perhaps use "~4.8" here, instead?

Comment thread README.md
This library provides provides a convient interface for developing Amazon Alexa Skills for your PHP app.
This library provides provides a convenient interface for developing Amazon Alexa Skills for your PHP app.

It represents a breaking change (and is forked) from jakobsuchy/amazon-alexa-php

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reword this statement

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.

2 participants