API Update API to suit its status as its own module #1
Conversation
GuySartorelli
force-pushed
the
pulls/1/new-repo
branch
3 times, most recently
from
ade52e6
to
dcb6a63
Compare
GuySartorelli
force-pushed
the
pulls/1/new-repo
branch
from
dcb6a63
to
809a652
Compare
| class SSViewer_BasicIteratorSupport implements TemplateIteratorProvider | ||
| class BasicIteratorSupport implements TemplateIteratorProvider |
There was a problem hiding this comment.
Removing SSViewer_ prefix from any classes in this repo. They don't have anything to do with SSViewer anymore.
There was a problem hiding this comment.
I'm keeping the SSTemplate prefix on classes in this repo which already had it because:
- It makes it instantly clear what kind of templates it's related to when used in project code
- It keeps things nicely tied together
- It removes the need to rename them, which (even if only slightly) means we're not introducing more upgrade pain.
.editorconfig
Outdated
There was a problem hiding this comment.
Copied from module standardiser, minus the bit about admin which isn't relevant here. Wanted it in here while I'm working to reduce cleanup required later.
phpcs.xml.dist
Outdated
| <!-- | ||
| <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 --> |
There was a problem hiding this comment.
Will clean this up before marking ready-to-merge.
There was a problem hiding this comment.
Should clean it up then as this PR's in review?
There was a problem hiding this comment.
Yes! Sorry. I thought I'd forget that's why I added that comment lol.
There was a problem hiding this comment.
Cleaned up, and resolved linting failures from a local run.
| if (defined('THIRDPARTY_PATH')) { | ||
| require_once(THIRDPARTY_PATH . '/php-peg/Parser.php'); | ||
| } else { | ||
| $base = dirname(__FILE__); | ||
| require_once($base.'/../thirdparty/php-peg/Parser.php'); | ||
| } | ||
| // Make sure to include the base parser code | ||
| $base = dirname(__FILE__); | ||
| require_once($base.'/../thirdparty/php-peg/Parser.php'); |
There was a problem hiding this comment.
That const was pointing to the thirdparty directory in framework, which isn't going to exist anymore anyway.
src/TemplateParser.php
Outdated
There was a problem hiding this comment.
Which this interface seems fairly generic on the surface, it's really only relevant for the parser used for parsing ss templates.
I thought about removing this abstraction but that's not really worth the effort right now. The benefit of doing so is basically nil and it introduces some non-zero chance of regressions.
phpcs.xml.dist
Outdated
| <!-- | ||
| <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 --> |
There was a problem hiding this comment.
Should clean it up then as this PR's in review?
This is a new repository.
The commit history on the
1branch is all of the commits fromsilverstripe/silverstripe-frameworkrelevant to the files we're migrating to this new module.This PR is for updating namespaces, class names, etc as appropriate, and for adding all the boilerplace (e.g. composer.json) required to turn this into a functioning Silverstripe CMS module.
CI
Since this is a new repository, CI won't run here. There is a CI run in the creative commoners fork, but that doesn't install because of a clash between this and the API in framework which hasn't been removed yet.
The CI won't even run in a dedicated CI run, because the matrix doesn't want to generate until this module is added to supported modules.
The kitchen sink run does include the unit tests for this module, which are in the recipe-core test suite in that run.
I recommend merging if that goes green. I'll still need to do some more PRs for this repo (e.g. run module standardiser) which will trigger subsequent CI for this repo itself, and we should be able to catch e.g. linting issues in those PRs.
Issue