NEW: Static validation for relationships. #9874

mfendeksilverstripe · 2021-02-26T02:37:39Z

Static validation for relationships

This is a new tool for static validation of relations. Very helpful if you are keen on keeping your relationship setup correct at all times. Especially useful for large projects with lot of models and relations.

Details

configurable (narrow down class inspection by namespace, class or/and relation names)
this feature is disabled by default and needs to be hooked up in order to work
two hooks are available: either unit tests or dev/build

madmatt · 2021-02-26T02:43:10Z

This seems really interesting - could we also add Documentation to the TODO. Also, if this is going into framework, my immediate thought is that it should be on by default (e.g. an explicit step in existing dev/build practices) and that it can be turned off if required (in practice by specifying disallowed_relationships for ones you're okay with not existing).

sminnee · 2021-02-26T04:04:31Z

Is there any reason we wouldn't enable this in dev/build by default? At least as a warning?

mfendeksilverstripe · 2021-03-08T19:06:35Z

@sminnee I would enable this by default in an ideal world but the reality is that many modules do not have properly setup relationships (back relation is either ambiguous or missing). This would force bespoke developers to fix configuration of modules they don't value that much. The biggest value we've got out of this tool was to validate the code we maintain.

Other reason for having this configurable is that some project may not care too much about relationship configuration. The main benefit is probably on larger projects where having properly defined relations allows:

more readable code / better navigation within the code and understanding in general
data reports relation functionality for example if you're looking into understanding where a certain model is referenced

@madmatt I'll review the configuration and check if I can provide a setup which can be enabled by default. The challenge here is to set it up in a way that bespoke code is covered but module code is not. This line is a bit blurry because for example you may want to target SiteTree as there may be bespoke extension on it.

sminnee · 2021-03-09T10:00:12Z

Another alternative would be to only throw a warning by default, so it shows up for those cases but doesn't break dev/build...

mfendeksilverstripe · 2021-03-09T17:42:59Z

Yeah, I think that's reasonable - warning for the dev/build and if you want to have a strict validation you can hook it up to your unit tests.

mfendeksilverstripe · 2021-03-23T19:12:28Z

@madmatt @sminnee I've pushed up new changes:

unit tests
docs
cleaner configuration API
hooked up to dev/build

Please review.

maxime-rainville

@silverstripe/core-team Before we do a detailed peer review on this, I just want to make sure we have an agreement in principle that this is a good idea worth merging.

I for one am in favour of shipping this feature as a warning that people can ignore. Maybe some projects can opt-in to fail their builds if this is something they care deeply about.

src/ORM/Validation/RelationValidationService.php

mfendeksilverstripe · 2021-04-06T21:50:25Z

@maxime-rainville Moved the code under Dev namespace.

emteknetnz · 2021-07-04T23:14:13Z

I like what this tool does, we just need to think about how this is triggered so that it's provides value and not just annoyance within peoples workflow

Currently it's enabled by default on

        if (static::class === \Page::class) {
            RelationValidationService::singleton()->devBuildCheck();
        }

Which means it's limited only regular app pages, though means that regular dataobjects in custom code aren't checked which will probably be the bulk of custom object for most projects. If find this pretty strange.

Some people will find this tool useful, for instance I can see it being great new projects where there's a strong focus on maintaining standards

For others it think this just unnecessary noise, such as dev being assigned to serveral years old project to do a sprints worth of work on a single feature. They're not going to get any value from being told there's a bunch of missing belongsTo relationships for parts of the system they're not working on.

I'd also expect that the deployment log on the majority of projects will have a bunch of warnings triggered, which is just going to be annoying noise. Myself coming mainly from a maintenance perspective I think in most cases this tool will be perceived as a negative if it's on by default

What I think would be useful is:

disabled by default so as not to annoy people on existing projects who probably don't want this feature
make this opt-in via:

a URL/CLI param to trigger as a one-off e.g. validate=1
project level config so that it's always on - this way it will show in deployment logs
a dev task provides a exit code so that it's failable within a CI context

clear, prominent docs for how to enable this
clear docs on how to limit this to the My/App/Namespace, because we really don't want to running on the vendor folder
ensure that this also works for module development as well as bespoke development

mfendeksilverstripe · 2021-07-04T23:52:39Z

Hey @emteknetnz , thanks for feedback. Some clarifications:

Which means it's limited only regular app pages, though means that regular dataobjects in custom code aren't checked which will probably be the bulk of custom object for most projects. If find this pretty strange.

The check condition is limited to the Page class only because we want to run it just once but the validation is not limited to page class, this is just how it's triggered. The service has it's own configuration and will check any classes available in the project (provided the configuration allows it).

The rest of the suggestions are very reasonable. I can definitely see how this can produce a lot of uneeded noise on older projects but it's really useful for new ones.

Setting the right defaults is the key. Happy to make adjustments as required 👍

emteknetnz · 2021-07-05T00:34:57Z

this is just how it's triggered

Ah ok, I see how this works now.

Possible the trigger point being in requireDefaultRecords is a bit odd . It may make sense to move the trigger point to a class that implements flushable - I think flush makes more sense rather than dev/build as $belongTo relationships won't create an database columns, though you may have a different opinion.

mfendeksilverstripe · 2021-10-20T19:22:27Z

This is ready for a review @emteknetnz .

emteknetnz

Good contribution. Needs to be opt-in configurable though.

The CLI output on dev/build flush is a little suboptimal, on my local it looks like this:

 * SilverStripe\ElementalFileBlock\Block\FileBlock
 * SilverStripe\ElementalBannerBlock\Block\BannerBlock
 * Page
 * BlocksPage
 * SilverStripe\ErrorPage\ErrorPage
  * RelationValidationService : 2 issues found (listed below)
  * Page / MyFile : Back relation not found or ambiguous (needs class.relation format)
  * BlocksPage / ElementalArea : Back relation not found or ambiguous (needs class.relation format)
  + 404 error page refreshed
  * RelationValidationService : 2 issues found (listed below)
  * Page / MyFile : Back relation not found or ambiguous (needs class.relation format)
  * BlocksPage / ElementalArea : Back relation not found or ambiguous (needs class.relation format)
  + 500 error page refreshed
 * SilverStripe\CMS\Model\RedirectorPage
 * SilverStripe\CMS\Model\VirtualPage

The RelationValidationService is kind of jammed in the middle of ErrorPage being built and it's indentation doesn't seem right

src/Dev/Validation/RelationValidationService.php

emteknetnz · 2022-01-20T23:05:36Z

src/Dev/Validation/RelationValidationService.php

+    public function inspectClasses(array $classes): array
+    {
+        self::reset();
+        $this->ignoreConfig = true;


should $this->ignoreConfig = false; be called after calling $this->validateClasses($classes);` ?

It needs to be called before the validation as configuration potentially narrows down the scope of the covered classes. This is meant to be used for debugging / figuring out configuration.

Understand that, I'm just wondering if there's a need to revert it to false afterwards, or if because it's just a "one-off run" per flush request it can just stay set to true

Reset to previous state of this flag is covered by self::reset() so there is no need to set it back.

src/Dev/Validation/RelationValidationService.php

mfendeksilverstripe · 2022-01-23T20:56:27Z

@emteknetnz I've pushed up changes as requested:

fixed a few typos
some linting fixes
feature is now opt-in
error messages are now formatted as errors

I'm not sure how to improve the dev/build output though. I can see that the validation runs twice during dev/build as well:

The output looks correct when running flush via dev/tasks though:

emteknetnz

Cheers. Still need to default the config to off + document

Is there anyway we can get the output to not be done seemingly at the same time as the Error pages being created when running dev/build flush ?

I've debugged this and the reasons it's showing here on dev/build flush before each of the 404 + 500 pages is:

ErrorPage::requireDefaultRecords()
ErrorPage::requireDefaultRecordFixture()
ErrorPage::writeStaticPage()
Director::test()

Presumably the Director::test() is used to generate HTML output for the error pages using templates etc.

I'm not sure how, though ideally we'd want to defer turning on / running this flushable service until after everything else

emteknetnz · 2022-01-25T21:49:35Z

src/Dev/Validation/RelationValidationService.php

+    public function inspectClasses(array $classes): array
+    {
+        self::reset();
+        $this->ignoreConfig = true;


Understand that, I'm just wondering if there's a need to revert it to false afterwards, or if because it's just a "one-off run" per flush request it can just stay set to true

emteknetnz · 2022-01-25T21:53:41Z

src/Dev/Validation/RelationValidationService.php

+     *
+     * @var bool
+     */
+    private static $flush_output_enabled = true;


Suggested change

private static $flush_output_enabled = true;

private static $flush_output_enabled = false;

Need to default this to off otherwise it'll be very annoying for the majority of people

Whoops, I left this value there from my testing run.

docs/en/02_Developer_Guides/00_Model/02_Relations.md

emteknetnz · 2022-01-25T21:55:38Z

src/Dev/Validation/RelationValidationService.php

+     */
+    public function executeValidation(): void
+    {
+        if (!$this->config()->get('flush_output_enabled')) {


This config check should be in the flush() method. The idea with the config is that it disables automatic running on every flush. However if we have the config here, we cannot $service = new RelationValidationService(); $service->executeValidation(); which someone might want to do as a one-off for a custom linting setup

emteknetnz · 2022-01-25T22:39:40Z

src/Dev/Validation/RelationValidationService.php

+     */
+    protected $errors = [];
+
+    public function flushErrors(): void


Suggested change

public function flushErrors(): void

public function clearErrors(): void

Probably best to avoid the word 'flush' since this is already a 'flusable' class

emteknetnz · 2022-01-25T22:39:50Z

src/Dev/Validation/RelationValidationService.php

+    public static function reset(): void
+    {
+        $service = self::singleton();
+        $service->flushErrors();


Suggested change

$service->flushErrors();

$service->clearErrors();

mfendeksilverstripe · 2022-02-02T19:31:22Z

I've pushed up new changes @emteknetnz :

several of your suggestions (docs update, minor code cleanup)
I couldn't find a god way how to execute the validation via Flush so I added a couple of extension point to the dev build process instead

emteknetnz

Cool, looking much better. No longer runs by default and output is no longer happening at the same time as Errorpage creation

dev/build flush

...
 * BlocksPage
 * SilverStripe\ErrorPage\ErrorPage
  + 404 error page refreshed
  + 500 error page refreshed
 * SilverStripe\CMS\Model\RedirectorPage
 * SilverStripe\CMS\Model\VirtualPage

 Database build completed!

  ! RelationValidationService : 2 issues found (listed below)
  ! Page / MyFile : Back relation not found or ambiguous (needs class.relation format)
  ! BlocksPage / ElementalArea : Back relation not found or ambiguous (needs class.relation format)

* NEW: Static validation for relationships. * Unit tests added. * PR fixes * PR feedback: Execute validation on flush. * PR fixes. * PR fixes.

NEW: Static validation for relationships.

6b8ea5d

mfendeksilverstripe force-pushed the feature/static-relations-validation branch 2 times, most recently from f19d0cf to c0871ee Compare March 23, 2021 02:37

Unit tests added.

b7b7c09

mfendeksilverstripe force-pushed the feature/static-relations-validation branch from c0871ee to b7b7c09 Compare March 23, 2021 18:46

brynwhyman assigned maxime-rainville Mar 29, 2021

maxime-rainville reviewed Apr 5, 2021

View reviewed changes

src/ORM/Validation/RelationValidationService.php Outdated Show resolved Hide resolved

PR fixes

ce576ca

mfendeksilverstripe force-pushed the feature/static-relations-validation branch from db3a158 to ce576ca Compare April 6, 2021 21:49

brynwhyman unassigned maxime-rainville Jun 29, 2021

emteknetnz mentioned this pull request Jul 4, 2021

Static validation for relationships #10008

Closed

emteknetnz linked an issue Jul 4, 2021 that may be closed by this pull request

Static validation for relationships #10008

Closed

PR feedback: Execute validation on flush.

cd3ecd8

mfendeksilverstripe force-pushed the feature/static-relations-validation branch from 07aff3a to cd3ecd8 Compare October 20, 2021 19:21

mfendeksilverstripe requested a review from maxime-rainville October 20, 2021 19:22

emteknetnz requested changes Jan 20, 2022

View reviewed changes

PR fixes.

7b7b95e

emteknetnz requested changes Jan 25, 2022

View reviewed changes

emteknetnz reviewed Jan 25, 2022

View reviewed changes

PR fixes.

db4c90d

emteknetnz approved these changes Feb 4, 2022

View reviewed changes

emteknetnz merged commit 89c87dd into silverstripe:4 Feb 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW: Static validation for relationships. #9874

NEW: Static validation for relationships. #9874

mfendeksilverstripe commented Feb 26, 2021 •

edited by emteknetnz

Loading

madmatt commented Feb 26, 2021

sminnee commented Feb 26, 2021

mfendeksilverstripe commented Mar 8, 2021 •

edited

Loading

sminnee commented Mar 9, 2021

mfendeksilverstripe commented Mar 9, 2021

mfendeksilverstripe commented Mar 23, 2021

maxime-rainville left a comment

mfendeksilverstripe commented Apr 6, 2021 •

edited

Loading

emteknetnz commented Jul 4, 2021 •

edited

Loading

mfendeksilverstripe commented Jul 4, 2021

emteknetnz commented Jul 5, 2021 •

edited

Loading

mfendeksilverstripe commented Oct 20, 2021

emteknetnz left a comment •

edited

Loading

emteknetnz Jan 20, 2022

mfendeksilverstripe Jan 23, 2022

emteknetnz Jan 25, 2022

mfendeksilverstripe Feb 2, 2022

mfendeksilverstripe commented Jan 23, 2022

emteknetnz left a comment •

edited

Loading

emteknetnz Jan 25, 2022

emteknetnz Jan 25, 2022

mfendeksilverstripe Feb 2, 2022

emteknetnz Jan 25, 2022

emteknetnz Jan 25, 2022

emteknetnz Jan 25, 2022

mfendeksilverstripe commented Feb 2, 2022

emteknetnz left a comment

	private static $flush_output_enabled = true;
	private static $flush_output_enabled = false;

	public function flushErrors(): void
	public function clearErrors(): void

NEW: Static validation for relationships. #9874

NEW: Static validation for relationships. #9874

Conversation

mfendeksilverstripe commented Feb 26, 2021 • edited by emteknetnz Loading

Static validation for relationships

Details

madmatt commented Feb 26, 2021

sminnee commented Feb 26, 2021

mfendeksilverstripe commented Mar 8, 2021 • edited Loading

sminnee commented Mar 9, 2021

mfendeksilverstripe commented Mar 9, 2021

mfendeksilverstripe commented Mar 23, 2021

maxime-rainville left a comment

Choose a reason for hiding this comment

mfendeksilverstripe commented Apr 6, 2021 • edited Loading

emteknetnz commented Jul 4, 2021 • edited Loading

mfendeksilverstripe commented Jul 4, 2021

emteknetnz commented Jul 5, 2021 • edited Loading

mfendeksilverstripe commented Oct 20, 2021

emteknetnz left a comment • edited Loading

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

mfendeksilverstripe commented Jan 23, 2022

emteknetnz left a comment • edited Loading

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

mfendeksilverstripe commented Feb 2, 2022

emteknetnz left a comment

Choose a reason for hiding this comment

mfendeksilverstripe commented Feb 26, 2021 •

edited by emteknetnz

Loading

mfendeksilverstripe commented Mar 8, 2021 •

edited

Loading

mfendeksilverstripe commented Apr 6, 2021 •

edited

Loading

emteknetnz commented Jul 4, 2021 •

edited

Loading

emteknetnz commented Jul 5, 2021 •

edited

Loading

emteknetnz left a comment •

edited

Loading

emteknetnz left a comment •

edited

Loading