doctor command removes existing htaccess rules

[emteknetnz]

Using 1.2.0

I ran upgrade-code doctor and it messed up .htaccess

It removed https redirection

<IfModule mod_headers.c>
    Header always append Vary "X-Forwarded-Proto" "expr=%{REQUEST_STATUS} == 301"
    Header always append Vary "X-Forwarded-Proto" "expr=%{REQUEST_STATUS} == 302"
...
    RewriteCond %{HTTPS} !on
    RewriteCond %{HTTP:X-Forwarded-Proto} !https [NC]
    RewriteCond %{HTTP_HOST} ^www.mysite.com [NC]
    RewriteRule ^(.*)$ https://www.mysite.com/$1 [R,L]

which is the recommended method of https redirection here https://www.cwp.govt.nz/developer-docs/en/2/how_tos/redirections

It removed some custom caching rules

    Header set Cache-Control "max-age=300, public"
    <filesMatch ".(jpg|jpeg|png|gif|ico)$">
        Header set Cache-Control "max-age=86400, public"
    </filesMatch>

    <filesMatch ".(css|js)$">
        Header set Cache-Control "max-age=7200, public"
    </filesMatch>

It removed a bunch rules to not allow access to files e.g.

<Files *.md>
    Order deny,allow
    Deny from all
    Allow from 127.0.0.1
</Files>

<Files Vagrantfile>
    Order deny,allow
    Deny from all
</Files>

Another few issues with doctor:

• it didn't dry run by default, it just ran i.e. didn't require me to --write like other commands
• it's weirdly named. most people will associate 'doctor' with the functionality of 'brew doctor' which does some analysis and tells you what's wrong, whereas this update-code doctor jumps in and modifies some files which was unexpected
• its description when you 'list' is "Run all cleanup tasks configured for this project" - it's not clear what the means

Pull requests

• [x] https://github.com/silverstripe/silverstripe-upgrader/pull/182
• [ ] https://github.com/silverstripe/silverstripe-framework/pull/9291

[dhensby]

I know this is unrelated to the problem, but those Vary append commands are very bad...

    Header always append Vary "X-Forwarded-Proto" "expr=%{REQUEST_STATUS} == 301"
    Header always append Vary "X-Forwarded-Proto" "expr=%{REQUEST_STATUS} == 302"

If the request can vary based on a header that should always be present in the response, not only when it's redirected. What the vary header signifies is that the response could vary on that header, not that it has varied.

[maxime-rainville]

The problem is probably not with doctor per say. Doctor will go through the modules and see if they define doctorTasks in .upgrade.yml which are essentially build tasks that are meant to help you migrate your project.

The doc for the command explains this, but it's easy to miss. https://github.com/silverstripe/silverstripe-upgrader/blob/master/docs/en/doctor.md

The upgrader don't control what those tasks do, so it can't show a diff and it can't can grantee it won't do destructive things.

In the specific case raise @emteknetnz, the problem is probably with one of doctor task. However doctor is bit of an odd command and could be improved a bit.

[chillu]

The upgrader don't control what those tasks do, so it can't show a diff and it can't can grantee it won't do destructive things.

Are you talking about the all-in-one upgrader you worked on @maxime-rainville? The reporter said "I ran upgrade-code doctor", so isn't using this. And I'd expect the upgrader can control what the upgrader does ;)

This is a bug, since it's violating the AC in the story card to implement this feature:

If I have a .htaccess file in the base path that's different from the 3.x defaults, I get a warning and instructions on how to migrate (link to contents of new file) The .htaccess file only contains relevant rules when placed in the public/ subfolder, removing unnecessary noise like RewriteRule ^vendor(/|$) - [F,L,NC]

The original story card also contained this AC:

https://github.com/silverstripe/silverstripe-upgrader/issues/65

The doctor docs are very opaque, they don't really mention what it does. The fact that you can hook in multiple tasks in this command is an implementation detail, the command should explain what it does to users.

[maxime-rainville]

@chillu the all command doesn't run doctor. I've excluded doctor on the basis that the help doc for it specifically say that you should avoid running it if you can.

What I mean by the The upgrader doesn't control what those tasks do is that doctor literally requires an arbitrary file from the provided module. The task could be doing whatever it wants from the upgrader's perspective.

It looks like we have only one doctorTask defined. It might be that we should just ditching this command as it doesn't provide much benefit and seem to be causing more problems than it solves.

[chillu]

I don't really care what runs which task where. The fact is that something in our upgrader tool is messing up .htaccess files, and that needs to be fixed.

[chillu]

Related: https://github.com/silverstripe/silverstripe-upgrader/issues/108

[chillu]

@maxime-rainville recommended to remove this task altogether. Max, can you please outline the tradeoffs?

[maxime-rainville]

Module can define doctor tasks in their .upgrade.yml file. These task are specific task that will be run by the doctor command.

Right now, only Framework defines one.

doctorTasks:
  SilverStripe\Dev\Upgrade\UpgradeBootstrap: src/Dev/Upgrade/UpgradeBootstrap.php

https://github.com/silverstripe/silverstripe-framework/blob/b2dd22fb5010f0baf6d577d9a118f37aa2c931b6/.upgrade.yml#L955-L956

The purpose of this task is to update the entry point of the application. This is already handled by the webroot command and the recipe-plugin. The UpgradeBootstrap implementation is a little less clever than the webroot command, because the webroot command will try to detect if you have made customisation before overwritting your files.

In this context UpgradeBootstrap and the doctor task looks like more trouble than they are worth.

[NightJar]

I just flipped my table because of this. I've painstakingly gone through the .htaccess in the public, updating it with all the project customisations and updates from newer version of recipe, only to have it all undone (pre-commit!) because this command does not behave like all the others - there is no -w flag, because this is the default.

It also overwrites index.php should any updates be applied (by a newer version of a recipe for example).

[maxime-rainville]

Just did two PRs:

• one against the upgrader that add a big nasty warning to the doctor command
• one that disables the framework task that seems to be tripping everyone up.