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

API Rename resources to _resources #8364

Closed
wants to merge 1 commit into from

Conversation

tractorcow
Copy link
Contributor

Fixes #7932

Requires silverstripe/vendor-plugin#25

Also all other modules will need to have their vendor-plugin dependency bumped to ^2 as well. My suggestion is to get the module merged first and migrate the modules one at a time.

@tractorcow
Copy link
Contributor Author

To discuss:

  • 4.x or 5.x?
  • soft-code or hard-code?
  • _resources as a default or something else?

@@ -55,8 +55,8 @@ protected function error($message = null)
protected function installHeader()
{
$clientPath = PUBLIC_DIR
? 'resources/vendor/silverstripe/framework/src/Dev/Install/client'
: 'resources/silverstripe/framework/src/Dev/Install/client';
? '_resources/vendor/silverstripe/framework/src/Dev/Install/client'
Copy link
Member

Choose a reason for hiding this comment

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

Why not use the constant for these bits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Laziness.

@ScopeyNZ
Copy link
Member

ScopeyNZ commented Sep 7, 2018

4.x or 5.x?

I don't see how this wouldn't be a breaking API change :( .

soft-code or hard-code?

+1 for making this name configurable in 5.x. Can we do some composer.json configuration to make that work?

@robbieaverill
Copy link
Contributor

I tend to agree with @ScopeyNZ. This will be very disruptive to our module ecosystem. Can we not do this but just make it configurable at the project level from SS5 on?

@tractorcow
Copy link
Contributor Author

Looks like we'll need to soft-code it. If it's not configured, it'll need to default to resources (per the constant), but we will probably need to do it in 5.x if existing 4.x code assumes the hard-coded value.

Copy link
Member

@chillu chillu left a comment

Choose a reason for hiding this comment

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

As discussed, should be configurable, so we can allow devs to change the value in 4.x, and reconfigure to a safer default in 5.x.

@maxime-rainville
Copy link
Contributor

#8364 should supersede this PR I think. Unless someone has objections, I'll close this one.

@sminnee
Copy link
Member

sminnee commented Nov 5, 2018

Closing in favour of #8519

@sminnee sminnee closed this Nov 5, 2018
@chillu chillu removed this from the Recipe 4.3.0 milestone Nov 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants