Make into vendormodule #91

andrewandante · 2017-11-17T12:08:10Z

For tidyness

dhensby · 2017-11-17T12:20:54Z

_config.php

-if (!defined('DEBUGBAR_DIR')) {
-    define('DEBUGBAR_DIR', 'debugbar');
+if (!Environment::getEnv('DEBUGBAR_DIR')) {
+    Environment::setEnv('DEBUGBAR_DIR', 'resources/lekoala/silverstripe-debugbar');


I think that this can be done more nicely with the resource locator...

Rather than setting an env var I'd be using the resource locator in all the place you need the path

This could be a fun afternoon...

andrewandante · 2017-11-17T17:40:18Z

Well, tests to fix, but that's the general idea.

andrewandante · 2017-11-20T16:45:33Z

Requires silverstripe/vendor-plugin#10 to fix the tests, cos ModuleLoader is awkward

tractorcow · 2017-11-20T21:39:56Z

code/Collector/ConfigCollector.php

@@ -53,8 +54,8 @@ public function getAssets()
        $name = $this->getName();

        return [
-            'base_path' => '/' . DEBUGBAR_DIR . '/javascript',
-            'base_url' => DEBUGBAR_DIR . '/javascript',
+            'base_path' => '/' . Director::makeRelative(ModuleLoader::getModule('lekoala/silverstripe-debugbar')->getResource('javascript')->getURL()),


Please use $resource->getRelativePath() instead of Director::makeRelative($resource->getURL())

I was having enormous trouble getting that to work as my dev environment lives on a sub-url, i.e.
http://dev/stabless4 - and I could never get the right path. This is the only one that worked. I will try again tomorrow but I want getRelativeURL() and there wasn't an option :)

Yes, but url isn't a path... you could have a resource that lives locally, but is served via a CDN, in which case there won't be any relation between the url and the path at all.

tractorcow · 2017-11-20T21:41:13Z

tests/Collector/SilverStripeCollectorTest.php

@@ -172,8 +171,8 @@ public function testGetWidgets()
    public function testGetAssets()
    {
        $expected = array(
-            'base_path' => '/debugbar/javascript',
-            'base_url' => 'debugbar/javascript',
+            'base_path' => '/resources/lekoala/silverstripe-debugbar/javascript',


Don't hard-code this assumption; Use the resources API.

A better test isn't an explicit hard-coded array result; It's that the path physically exists, and that the URL Is accessible.

I agree - I think I should move away from relying on the module being in a particular place.

Travis testing is a nightmare with this though - because it runs the composer install from inside the module, it means all the resources are in the wrong places. Will have a play again tomorrow - and see if I can figure out a standard for testing with modules that expose resources.

I agree and disagree - a known input should result in a known output for a test. whilst we don't want to tightly couple our tests to these exact values, these assumptions should still hold...

"installed in vendor or in root" isn't a known input it's a variable, and it shouldn't matter which it is.

$config = $this->collector->getAssets(); $this->assertDirectoryExists(BASE_PATH . '/' . $config['base_path]);

Otherwise, test for either of the two possible valid locations.

tractorcow · 2017-11-20T21:41:53Z

code/DebugBar.php

@@ -234,8 +235,8 @@ public static function includeRequirements()
        $renderer = $debugbar->getJavascriptRenderer();

        // We don't need the true path since we are going to use Requirements API that appends the BASE_PATH
-        $renderer->setBasePath(DEBUGBAR_DIR . '/assets');
-        $renderer->setBaseUrl(DEBUGBAR_DIR . '/assets');
+        $renderer->setBasePath(Director::makeRelative(ModuleLoader::getModule('lekoala/silverstripe-debugbar')->getResource('assets')->getURL()));


As above, use getPath() / getRelativePath() for a path, don't try to infer from getURL().

tractorcow · 2017-11-20T21:43:50Z

silverstripe/vendor-plugin#10 isn't the problem, I've commented to that effect on that PR.

@andrewandante do you want to chat offline about these changes? Maybe I can help you with the API issues.

tractorcow · 2017-11-20T21:45:03Z

code/Collector/DatabaseCollector.php

@@ -173,8 +173,8 @@ public function getWidgets()
    public function getAssets()
    {
        return array(
-            'base_path' => '/' . DEBUGBAR_DIR . '/javascript',
-            'base_url' => DEBUGBAR_DIR.'/javascript',
+            'base_path' => '/' . Director::makeRelative(ModuleLoader::getModule('lekoala/silverstripe-debugbar')->getResource('javascript')->getURL()),


Do you think you could consolidate this repeated config into a DebugBarConfig or something?

dhensby · 2017-11-21T16:43:41Z

tests/Collector/DatabaseCollectorTest.php


-class DatabaseCollectorTest extends SapphireTest
+class DatabaseCollectorTest extends FunctionalTest


Why do we need to use FunctionalTest now?

dhensby · 2017-11-21T16:43:52Z

tests/Collector/SilverStripeCollectorTest.php


-class SilverStripeCollectorTest extends SapphireTest
+class SilverStripeCollectorTest extends FunctionalTest


Why do we need to use FunctionalTest now?

dhensby · 2017-11-21T16:44:30Z

tests/Collector/SilverStripeCollectorTest.php

@@ -26,6 +27,12 @@ public function setUp()
        parent::setUp();
        DebugBar::initDebugBar();
        $this->collector = DebugBar::getDebugBar()->getCollector('silverstripe');
+        Requirements::set_backend(Requirements_Backend::create());


This looks like a regression in 4.0 - would you mind PR-ing to core to re-add this via a test state class or maybe adding to the sapphireteststate class?

Turns out it's not actually required - I think caching had something to do with it. Have removed, hopefully Travis still likes me

dhensby · 2017-11-21T16:44:39Z

tests/Collector/SilverStripeCollectorTest.php

+        Requirements::set_backend(Requirements_Backend::create());
+    }
+
+    public function tearDown()


this stub isn't needed

dhensby · 2017-11-21T16:45:22Z

tests/Collector/SilverStripeCollectorTest.php

@@ -167,16 +180,18 @@ public function testGetWidgets()
        );

        $this->assertSame($expected, $result);
+        $this->logOut();


tearDown should be doing logouts, so this shouldn't be needed

Only in the setUp() method of FunctionalTest - so as I will set them back to SapphireTest I'll leave this in there

fix the tests

andrewandante · 2017-12-05T11:48:55Z

ping @tractorcow

andrewandante force-pushed the make_into_vendormodule branch from 663a568 to cb9d5c0 Compare November 17, 2017 12:14

dhensby reviewed Nov 17, 2017

View reviewed changes

andrewandante force-pushed the make_into_vendormodule branch 3 times, most recently from 4f4cd47 to ed9248e Compare November 17, 2017 17:35

andrewandante force-pushed the make_into_vendormodule branch 24 times, most recently from 7f8b7bc to de2d2b2 Compare November 20, 2017 14:45

andrewandante force-pushed the make_into_vendormodule branch 5 times, most recently from 1b309d7 to 03ffdfa Compare November 20, 2017 16:06

tractorcow suggested changes Nov 20, 2017

View reviewed changes

tractorcow reviewed Nov 20, 2017

View reviewed changes

andrewandante force-pushed the make_into_vendormodule branch 7 times, most recently from 17caf83 to b210c64 Compare November 21, 2017 15:46

dhensby reviewed Nov 21, 2017

View reviewed changes

andrewandante force-pushed the make_into_vendormodule branch from b210c64 to a131941 Compare November 22, 2017 10:09

convert to vendormodule, expose assets and js

8968a74

fix the tests

andrewandante force-pushed the make_into_vendormodule branch from a131941 to 8968a74 Compare November 22, 2017 10:32

robbieaverill approved these changes Dec 5, 2017

View reviewed changes

robbieaverill merged commit 055c36a into lekoala:master Dec 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make into vendormodule #91

Make into vendormodule #91

andrewandante commented Nov 17, 2017

dhensby Nov 17, 2017

dhensby Nov 17, 2017

andrewandante Nov 17, 2017

andrewandante commented Nov 17, 2017

andrewandante commented Nov 20, 2017

tractorcow Nov 20, 2017

andrewandante Nov 20, 2017

tractorcow Nov 20, 2017

tractorcow Nov 20, 2017

tractorcow Nov 20, 2017

andrewandante Nov 20, 2017

dhensby Nov 20, 2017

tractorcow Nov 20, 2017

tractorcow Nov 20, 2017

tractorcow commented Nov 20, 2017

tractorcow Nov 20, 2017

dhensby Nov 21, 2017

dhensby Nov 21, 2017

dhensby Nov 21, 2017

andrewandante Nov 22, 2017

dhensby Nov 21, 2017

dhensby Nov 21, 2017

andrewandante Nov 22, 2017

dhensby Nov 22, 2017

andrewandante commented Dec 5, 2017


		class DatabaseCollectorTest extends SapphireTest
		class DatabaseCollectorTest extends FunctionalTest


		class SilverStripeCollectorTest extends SapphireTest
		class SilverStripeCollectorTest extends FunctionalTest

Make into vendormodule #91

Make into vendormodule #91

Conversation

andrewandante commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewandante commented Nov 17, 2017

andrewandante commented Nov 20, 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

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

tractorcow commented Nov 20, 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

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

andrewandante commented Dec 5, 2017