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

Add domain whitelisting #45

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

Conversation

guilebc
Copy link
Contributor

@guilebc guilebc commented Feb 22, 2017

Domain whitelisting feature for the Messenger Extensions & Plugin Checkbox.
https://developers.facebook.com/docs/messenger-platform/thread-settings/domain-whitelisting

Tell me if ok for you.

{
return [
DomainWhitelisting::WHITELISTED_DOMAINS => $this->domains,
DomainWhitelisting::DOMAIN_ACTION_TYPE => $this->action
Copy link
Owner

Choose a reason for hiding this comment

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

Are you sure this is needed ? No mention of that in the documentation.

*
* @return array
*/
private function buildSetting($type, $threadState = null, $value = null)
private function buildSetting($type, $threadState = null, $value = null, $mergeValueWithSetting = false)
Copy link
Owner

Choose a reason for hiding this comment

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

The documentation look like to be updated. No need to threadState and setting_type anymore. Maybe a rewrite of this function should be better.


use Tgallice\FBMessenger\Model\ThreadSetting\DomainWhitelisting;

class WhitelistedDomains
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure about the need of this class. Or It's should be more generic. Maybe rename DomainWhitelisting to DomainWhitelist and used it instead of WhitelistedDomains. The DomainWhitelist should contain only domain list.

@tgallice
Copy link
Owner

tgallice commented Mar 5, 2017

@guilebc sorry for the delais, just some comments but it's cool 👍 thank.

@Shaked
Copy link
Contributor

Shaked commented Aug 16, 2017

@tgallice - is there a plan to merge this pull request?

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.

3 participants