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 a clear task #40

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
version: ~> 1.0

import:
- silverstripe/silverstripe-travis-shared:config/provision/standard-jobs-range.yml
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ This module adds an implementation of [graphiql](https://github.com/graphql/grap

This is because GraphQL 4 has its own `DevelopmentAdmin` controller.

The GraphQL v4 version of the module allows you to clear your schema by calling the `/dev/graphql/clear` task.

## Security

By default, the tool has the same restrictions as other development tools like `dev/build`:
Expand Down Expand Up @@ -101,4 +103,3 @@ SilverStripe\Control\Director:
CDN to download the latest distribution and drop it into this repository. Be sure
to update the comment at the top of the `bundle.js` file to track the URL it was
downloaded from.

4 changes: 4 additions & 0 deletions _config/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ SilverStripe\GraphQL\Dev\DevelopmentAdmin:
controller: 'SilverStripe\GraphQLDevTools\Controller'
links:
ide: Run the GraphQL IDE
clear:
controller: SilverStripe\GraphQLDevTools\Clear
links:
clear: Clear the GraphQL schema

---
Name: graphql3-devtool-routes
Expand Down
15 changes: 10 additions & 5 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,23 @@
"name": "silverstripe/graphql-devtools",
"description": "Tools to help developers building new applications on SilverStripe’s GraphQL API",
"type": "silverstripe-vendormodule",
"license": "BSD-3-Clause",
"require": {
"silverstripe/graphql": "*"
"silverstripe/graphql": "^3 || ^4",
Copy link
Member

Choose a reason for hiding this comment

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

Does this actually work with graphql 3? I'm seeing imports in other files such as SilverStripe\GraphQL\Schema\Logger

Copy link
Member

Choose a reason for hiding this comment

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

The clear functionality doesn't work with v3 - but it also isn't available with v3 (see yml config)

Copy link
Member

Choose a reason for hiding this comment

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

Oh right but the rest of this module does work with graphql3? Just not clear?

Copy link
Member

Choose a reason for hiding this comment

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

I believe it does - it at least used to and is supposed to, and I don't think anything in PR would stop it from doing so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's meant to work with both of them. However, I'm thinking we should tag a v3 version and a v4 version to avoid confusion. I thought of doing it in this PR, but the travis build wanted to test both of them any way, so it was failing the build.

Copy link
Member

Choose a reason for hiding this comment

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

we should tag a v3 version and a v4 version to avoid confusion

If it's meant to have dual support, then why not just let it have dual support? Especially since at this point it will be more work to unpick it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the dual support is a bit awful, though pragmatically we should just leave as is. Perhaps do a new major at CMS 5 time that is graphql 4 only

"symfony/finder": "^4 || ^5",
"symfony/filesystem": "^4 || ^5"
},
"require-dev": {
"phpunit/phpunit": "^9.5",
"squizlabs/php_codesniffer": "^3.0"
},
"autoload": {
"psr-4": {
"SilverStripe\\GraphQLDevTools\\": "src/"
"SilverStripe\\GraphQLDevTools\\": "src/",
"SilverStripe\\GraphQLDevTools\\Tests\\": "tests/"
}
},
"extra": {
"branch-alias": {
"dev-master": "1.x-dev"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this being removed?

Copy link
Member

@emteknetnz emteknetnz May 19, 2022

Choose a reason for hiding this comment

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

There a history of these things being in Silverstripe modules, they're generally out of date so more annoying than helpful. These days we remove these things any time we see them. I don't think this needs a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Oh there's no 1 branch and no stable tags. I guess if someone out there is requiring this as ^1 then that'll break.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs a separate PR

I agree it's okay in this PR so long as we can say why it's happening, and if it should happen. Which you've now provided! Thanks.
In this case I'd say it's okay. We're not even tagging this module, so anyone with ^1 as a constraint with this module imo has the wrong constraint, and it should break for them, as a sanity check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My instinct would be to start tagging release numbers for this.

},
"expose": [
"client/"
]
Expand Down
28 changes: 28 additions & 0 deletions phpcs.xml.dist
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?xml version="1.0" encoding="UTF-8"?>
<ruleset name="SilverStripe">
<description>CodeSniffer ruleset for SilverStripe coding conventions.</description>

<file>src</file>
<file>tests</file>

<exclude-pattern>*/\.graphql-generated/*</exclude-pattern>

<!-- base rules are PSR-2 -->
<rule ref="PSR2" >
<!-- Current exclusions -->
<exclude name="PSR1.Methods.CamelCapsMethodName" />
<exclude name="PSR1.Files.SideEffects.FoundWithSymbols" />
<exclude name="PSR2.Classes.PropertyDeclaration" />
<exclude name="PSR2.ControlStructures.SwitchDeclaration" /> <!-- causes php notice while linting -->
<exclude name="PSR2.ControlStructures.SwitchDeclaration.WrongOpenercase" />
<exclude name="PSR2.ControlStructures.SwitchDeclaration.WrongOpenerdefault" />
<exclude name="PSR2.ControlStructures.SwitchDeclaration.TerminatingComment" />
<exclude name="PSR2.Methods.MethodDeclaration.Underscore" />
<exclude name="Squiz.Scope.MethodScope" />
<exclude name="Squiz.Classes.ValidClassName.NotCamelCaps" />
<exclude name="Generic.Files.LineLength.TooLong" />
<exclude name="PEAR.Functions.ValidDefaultValue.NotAtEnd" />
</rule>

</ruleset>

18 changes: 18 additions & 0 deletions phpunit.xml.dist
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<phpunit bootstrap="vendor/silverstripe/framework/tests/bootstrap.php" colors="true">

<testsuites>
<testsuite name="Default">
<directory>tests</directory>
</testsuite>
</testsuites>

<filter>
<whitelist addUncoveredFilesFromWhitelist="true">
<directory suffix=".php">src/</directory>
<exclude>
<directory suffix=".php">tests/</directory>
</exclude>
</whitelist>
</filter>

</phpunit>
49 changes: 49 additions & 0 deletions src/Clear.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?php
namespace SilverStripe\GraphQLDevTools;

use Psr\Log\LoggerInterface;
use SilverStripe\Control\Controller;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Core\Injector\Injector;
use SilverStripe\GraphQL\Schema\Storage\CodeGenerationStore;
use Symfony\Component\Filesystem\Filesystem;
use Symfony\Component\Finder\Finder;

class Clear extends Controller
{
private static $url_handlers = [
'' => 'clear',
];

private static $allowed_actions = [
'clear',
];

public function clear(HTTPRequest $request): void
{
$logger = Injector::inst()->get(LoggerInterface::class . '.graphql-build');
$dirName = CodeGenerationStore::config()->get('dirName');
$expectedPath = BASE_PATH . DIRECTORY_SEPARATOR . $dirName;
$fs = new Filesystem();

$finder = new Finder();
// Make finder not recursive
$finder->depth('== 0');

$logger->info('Clearing GraphQL code generation directory');
if ($fs->exists($expectedPath)) {
$logger->info('Directory has been found');
if ($finder->in($expectedPath)->hasResults()) {
foreach ($finder as $file) {
$logger->info('Removing ' . $file->getFilename());
$fs->remove($file->getRealPath());
}
$logger->info('Directory now empty');
} else {
$logger->info('Directory is already empty.');
}
} else {
$logger->info('Directory was not found. There is nothing to clear');
}
}
}
1 change: 0 additions & 1 deletion src/Controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ protected function findAvailableRoutes($schemas = []): array
}
} catch (InjectorNotFoundException $ex) {
}

}
return $routes;
}
Expand Down
68 changes: 68 additions & 0 deletions tests/ClearTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<?php

namespace SilverStripe\GraphQLDevTools\Tests;

use SilverStripe\Dev\FunctionalTest;
use SilverStripe\GraphQL\Schema\Storage\CodeGenerationStore;
use Symfony\Component\Filesystem\Filesystem;
use Symfony\Component\Finder\Finder;
use SilverStripe\GraphQL\Schema\Logger;
use SilverStripe\GraphQL\Schema\Schema;

class ClearTest extends FunctionalTest
{

private $originalDirName;
private $dirName = 'test-graphql-generated';
private $absDirName = BASE_PATH . DIRECTORY_SEPARATOR . 'test-graphql-generated';

protected $usesDatabase = true;

protected function setUp(): void
{
parent::setUp();
if (!class_exists(Schema::class)) {
$this->markTestSkipped('GraphQL 4 test ' . __CLASS__ . ' skipped');
return;
}

$this->originalDirName = CodeGenerationStore::config()->get('dirName');
Logger::singleton()->setVerbosity(Logger::EMERGENCY);
CodeGenerationStore::config()->set('dirName', $this->dirName);

$fs = new Filesystem();
$fs->mkdir($this->absDirName);
}

protected function tearDown(): void
{
parent::tearDown();
CodeGenerationStore::config()->set('dirName', $this->originalDirName);
$fs = new Filesystem();
if ($fs->exists($this->absDirName)) {
$fs->remove($this->absDirName);
}
}

public function testClear()
{
$fs = new Filesystem();
$finder = new Finder();
$finder->in($this->absDirName);

$this->assertTrue($fs->exists($this->absDirName), 'Test should begin with a fake code gen folder');

$this->get('dev/graphql/clear');
$this->assertFalse($finder->hasResults(), 'GraphQL clear should not break on an empty folder');

$fs->mkdir($this->absDirName . DIRECTORY_SEPARATOR . 'default');
$this->assertTrue($finder->hasResults(), 'A fake schema folder should have been created');

$this->get('dev/graphql/clear');
$this->assertFalse($finder->hasResults(), 'GraphQL clear should have removed the fake schema folder');

$fs->remove($this->absDirName);
$this->get('dev/graphql/clear');
$this->assertFalse($fs->exists($this->absDirName), 'GraphQL clear should not break on a non-existent folder');
}
}