BUG Remove broken RESOURCES_DIR test

[maxime-rainville]

That test is no longer needed because we won't allow you to save a page with an underscore in its URLSegment.

This should fix the broken recipe-cms build and some of the sink build.

[dhensby]

How do we prevent the use of underscores? And why do we prevent that? Wouldn't someone be able to change the URL substitutions so that underscores are used instead of hyphens?

[maxime-rainville]

Just double checked in SS45 and the CMS won't let me create a page called _resources even thought this test passes. I'll have a look to see if I can pinpoint the commit that broke the test, but in practice the UI has been disallowing underscores for some time.

[maxime-rainville]

I think that might be the thing that broke it silverstripe/silverstripe-framework#9064

[maxime-rainville]

Actually, the "problem" is the new DataObject hydration logic. silverstripe/silverstripe-framework#8591

My test was abusing the DataObject constructor to initialise the URLSegment. This was also setting the original values on the DataObject which caused SiteTree::onBeforeWrite() to skip a bunch of logic meant to normalise the URLSegment because it didn't think the URL had changed.

Sam's PR makes my dodgy constructor call actually work in a sensible way ... which breaks the test. Since the situation I was testing for can not happen in practice, I think removing the test is the right thing to do.

[emteknetnz]

What happens if RESOURCES_DIR == 'resources' instead of '_resources' ?

Plenty of existing sites will still have the old fashioned 'resources' directory in play

[maxime-rainville]

That scenario is handle just below the test I'm deleting. That is the default value.

If you start a new project from the installer the default your resource dir is overridden to _resources in your composer file.

[emteknetnz]

        if (RESOURCES_DIR !== 'resources') {
            $this->markTestSkipped('This legacy test requires RESOURCES_DIR to be "resources"');
        }

Just need to confirm that this test isn't going to get skipped

That is the default value.

So to confirm, 'resources' is the default value of RESOURCES_DIR? Or is it '_resources' (which is what I would expect) ?

[maxime-rainville]

When SS4.0 was released, vendor-expose would put static assets in resources which was a dumb idea because it prevents us from using resources as a URL Segment. So around 4.3, we added a composer flag to allow projects to name the folder something else. New project automatically get the composer flag set to _resources. Default value without the flag is still ressources because we were afraid of breaking existing projects.

Because RESOURCES_DIR get defined the project is bootstrapped, there's no way to changed the value dynamically in the tests.

testLegacyResourcesDirValuesHaveIncrementedValueAppended will get run on the CMS build because the module composer file won't have a resources-dir flag.

testDefaultResourcesDirHasLeadingUnderscoreRemovedAndResourcesIsUsed will be run the installer and recipe-cms builds because their composer files have a resources-dir flag.

[emteknetnz]

@maxime-rainville OK sounds good, could you put an inline comment within the unit tests file. All good if you just copy paste your last comment straight in

[maxime-rainville]

Done