Sachin/pi 1200 #150

manatarms · 2017-07-13T17:59:11Z

Updated cloudflare-plugin-backend to 2.2.0
Implemented getConfig and updated related files
Some formatting thing
Added sample json

jwineman · 2017-07-13T18:01:12Z

src/Cpanel/PluginActions.php

+        'featureManagerIsFullZoneProvisioningEnabled' => true,
+        'isDNSPageEnabled' => true,
+        'useHostAPILogin' => true,
+        'homePageCards' => array(


FYI you could just do:

array('AlwaysOnlineCard','IPV6Card',...,'PurgeCacheCard');

Don't need to change but in the future lets avoid hard coding indexes.

jwineman

LGTM but make sure @thellimist approves before merging.

thellimist · 2017-07-13T23:25:53Z

src/Cpanel/PluginActions.php

+        $this->getComposerJson();
+
+        //Clone the config to manipulate
+        $config = array_merge(array(), self::$CONFIG);


I'm not sure if array_merge does deep copy. You might need to use array_merge_recursive.

I think you would only need to do this if you weren't merging into an empty array(). None of the child arrays will conflict since they don't exist on the empty array()

Basically I think you're right but since we're using array_merge() to clone an array it doesn't matter.

thellimist · 2017-07-13T23:33:31Z

src/Cpanel/PluginActions.php

+                $this->userConfig = [];
+            } else {
+                $this->userConfig = json_decode($userConfigContent, true);
+            }


else case is almost never needed. This can be written as

$this->userConfig = []; if ($userConfigContent) { $this->userConfig = json_decode($userConfigContent, true); }

thellimist · 2017-07-13T23:36:43Z

src/Test/Cpanel/PluginActionsTest.php

+        ];
+        $this->pluginActions->setUserConfig($userConfig);
+        $response = $this->pluginActions->getConfig();
+        $this->assertEquals(true, $response['debug']);


assertTrue can be used

thellimist · 2017-07-13T23:43:16Z

config.json.sample

@@ -0,0 +1,3 @@
+{
+    "featureManagerIsFullZoneProvisioningEnabled": false
+}


@jwineman we can't write comments on .json files. We need to let the user figure out what settings are configurable. From this sample file as a user I'd say only featureManagerIsFullZoneProvisioningEnabled is configurable.

In my opinion, we should have all the default settings which can be configurable in the sample. I've listed the items which I think should be configurable.

{ "debug": false, "featureManagerIsFullZoneProvisioningEnabled": true, "homePageCards": [ "AlwaysOnlineCard", "IPV6Card", "CacheLevelCard", "RailgunCard", "PurgeCacheCard" ], "moreSettingsCards": { "container.moresettings.security": [ "SecurityLevelCard", "ChallengePassageCard", "BrowserIntegrityCheckCard" ], "container.moresettings.speed": [ "MinifyCard", "DevelopmentModeCard", "BrowserCacheTTLCard" ] }, "locale": "en", }

Sure, lets put it in the README instead of the sample. I don't want the sample merging duplicate keys every time if we can avoid it.

I think the only ones they should care about are debug, locale, and featureManagerIsFullZoneProvisioningEnabled though. We don't want them overwriting the cards but we don't necessarily care if they do.

If debug, locale and featureManagerIsFullZoneProvisioningEnabled will be configurable I think we should add all of them in the sample. It provides an ease of mind to the user when it's already there with default settings.

Additionally, we can make the README more explicit.

If we only add those three to the sample we don't need to make the README more explicit.

@manatarms sorry to make you change again. Could you only add those 3 settings in the sample.

Thanks

thellimist · 2017-07-13T23:49:18Z

src/Cpanel/PluginActions.php

+        ),
+        'locale' => 'en',
+        'integrationName' => 'cpanel',
+    );


We don't want all of the fields to be configurable. I think the below fields should not be configurable by the config.json file.

'isDNSPageEnabled' => true, 'useHostAPILogin' => true, 'integrationName' => 'cpanel',

@jwineman I just wrote the settings I think which shouldn't be configurable. I only have strong opinions on integrationName and useHostAPILogin. isDNSPageEnabled could be configurable...

thellimist · 2017-07-13T23:58:46Z

src/Cpanel/PluginActions.php

-            } else {
+            $this->userConfig = [];
+            //If we did find a config decode it
+            if ($userConfigContent) {


Don't comment what is obvious like

//The width of the line is 2 lineWidth = 2;

Write comments explaining "why" rather than "what" and only write it when it's needed. The "what" should be understandable by reading the code itself

manatarms · 2017-07-14T18:24:16Z

@thellimist @jwineman Made all the changes. Let me know if it's good to merge.

thellimist · 2017-07-14T19:45:42Z

src/Cpanel/PluginActions.php

@@ -16,33 +16,39 @@ class PluginActions extends AbstractPluginActions
    protected $userConfig;
    protected $composer;

-    public static $CONFIG = array(


As of PHP 5.4 you can also use the short array syntax, which replaces array() with [].

from http://php.net/manual/en/language.types.array.php

In WordPress we support PHP 5.3.10. We need to use array() rather than []

I just realized this is CPanel 😄. What is the CPanel min PHP version we support?

I just realized this wasn't the backend. cPanel is pinned to 5.6, we can use short array syntax.

jwineman · 2017-07-14T20:01:22Z

Make sure we include the changes from #149 here.

manatarms · 2017-07-14T20:57:26Z

Okay this seems good to go! Let me know if you'll are cool with the merge.

manatarms requested review from thellimist and jwineman July 13, 2017 17:59

jwineman reviewed Jul 13, 2017

View reviewed changes

jwineman approved these changes Jul 13, 2017

View reviewed changes

thellimist reviewed Jul 13, 2017

View reviewed changes

jwineman approved these changes Jul 14, 2017

View reviewed changes

thellimist suggested changes Jul 14, 2017

View reviewed changes

thellimist approved these changes Jul 14, 2017

View reviewed changes

manatarms added 9 commits July 14, 2017 13:50

Updated cloudflare-plugin-backend

abbfcde

Implemented getConfig and updated related files

a4f3ea2

Some formatting thing

35b285a

Added sample json! Yay

378f836

Updated README and removed array indexes

854bf4d

Requested changes and returned successAPIresponse

057066f

PI-1200 Linted files

975afc3

PI-1200 Blacklisted some keys

7767592

PI-1200 fixed conflict

565de4e

manatarms force-pushed the sachin/PI-1200 branch from e85752e to 565de4e Compare July 14, 2017 20:55

thellimist approved these changes Jul 14, 2017

View reviewed changes

manatarms merged commit 6a372d7 into master Jul 14, 2017

manatarms deleted the sachin/PI-1200 branch July 14, 2017 21:29

thellimist mentioned this pull request Aug 30, 2017

Cloudflare installer says: /scripts/postupcp: No such file or directory #158

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sachin/pi 1200 #150

Sachin/pi 1200 #150

manatarms commented Jul 13, 2017

jwineman Jul 13, 2017

manatarms Jul 13, 2017

jwineman left a comment

thellimist Jul 13, 2017

jwineman Jul 13, 2017

jwineman Jul 13, 2017

thellimist Jul 13, 2017

thellimist Jul 13, 2017

thellimist Jul 13, 2017

jwineman Jul 13, 2017

jwineman Jul 13, 2017

thellimist Jul 13, 2017

jwineman Jul 14, 2017

thellimist Jul 14, 2017

thellimist Jul 13, 2017

thellimist Jul 13, 2017

manatarms commented Jul 14, 2017

thellimist Jul 14, 2017

jwineman Jul 14, 2017

thellimist Jul 14, 2017

jwineman Jul 14, 2017

manatarms Jul 14, 2017

jwineman commented Jul 14, 2017

manatarms commented Jul 14, 2017

Sachin/pi 1200 #150

Sachin/pi 1200 #150

Conversation

manatarms commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwineman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manatarms commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwineman commented Jul 14, 2017

manatarms commented Jul 14, 2017