silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 821 forks source link

/assets/ error pages not regenerating #8858

Open albanyacademy opened 5 years ago

albanyacademy commented 5 years ago

Affected Version

Silverstripe 4.3.1/2 (these are the only two i've tested)

Description

pages error-404 and error-500.html (automatically generated by silverstripe) fail to use changes to the templates and will stay with whatever version they are until they're deleted. e.g. code changes to page.ss or to files used by Requirements:: will be correct and up-to-date when visiting any other bad URL on the site, but any wrong URL under /assets/ will use the html pages, which will be stuck on whatever their first configuration is. If installed without the Simple theme, these pages will use debug.css.

Steps to Reproduce

On Windows machine:

  1. composer create-project silverstripe/installer --prefer-dist .
  2. Complete install steps and do not use the Simple theme.
  3. Create a Page.ss with literally anything in it under /app/templates/
  4. Delete public/assets/error-404.html
  5. Run dev/build?flush=1
  6. Attempt to visit localhost/sitename/assets/test.pdf - should see the same template in Page.ss
  7. Change Page.ss template
  8. Run dev/build?flush=1
  9. Attempt to visit localhost/sitename/assets/test.pdf (or test2.pdf if it's cached by the browser) - should not see new changes applied.
  10. edit .css file used by Requirements e.g.

    parent::init();
    
            $loader = SilverStripe\View\ThemeResourceLoader::inst();
            $themes = SilverStripe\View\SSViewer::get_themes();
    
            $styles = [
                $loader->findThemedCSS("typography.min.css", $themes),
                $loader->findThemedCSS("someother.min.css", $themes),
                $loader->findThemedCSS("components.min.css", $themes)
            ];
            Requirements::combine_files("common.css", $styles);
  11. run dev/build?flush again
  12. view bogus url, observe that 404 page is referencing an outdated combinedfiles URL
albanyacademy commented 5 years ago

i can understand why assets would have PHP disabled within, though it would be nice for /assets/ to use the same error responses as the rest of the site. This makes it difficult to apply things like redirects - e.g. an old site on 3.7x that had external backlinks to a PDF uploaded through the CMS now needing a 301 in place for the new site... can sort of handle these issues when they crop up in the resources folder by modifying the /resources/.htaccess to process through index.php rather than blocking 404s.

dnsl48 commented 5 years ago

The current error-404.html and error-500.html are generated once by the dev/build as a fall back for the assets folder by the https://github.com/silverstripe/silverstripe-errorpage module.

I'm not sure the dev/build should regenerate those by default, as this goes a little beyond the application responsibilities. Depending on the server setup, you may actually want not to have those files in there and change .htaccess to direct 404 to something else or even serve them through a CDN.

However, I think we should expose another dev/... task which could be triggered manually to regenerate those files on demand.

albanyacademy commented 5 years ago

If every other file in the /assets/ folder gets rebuilt on every dev/build by default, why not the error pages as well? Exposing a separate dev task is nice, but having those error pages be 1:1 with the PHP-served error pages, in my mind, should be out-of-the-box.

dnsl48 commented 5 years ago

If every other file in the /assets/ folder gets rebuilt on every dev/build by default

sorry, I'm not exactly sure what you mean by that. I believe the main dev/build purpose is to keep the database structure in sync with the ORM, akin to database migrations. While doing so, some of the ORM entities may perform side effects. One of those is the errorpage generating these two error page assets (404 and 500). I think that's the main reason this 2 files aren't being regenerated - because the ORM entity has already been created in the database.

I'm not sure such side effects are ideal, and should be attached to database migrations.

Regarding the particular problem you reported, there's been a discussion, which led to moving the errorpage to a separate module, letting people extend the default error page behaviour.

albanyacademy commented 5 years ago

dev/build regenerates htaccess file among others on every dev/build. Seems strange that static 404 and 500 aren't included in this behaviour, and are only treated as one-offs. Is there a reason behind this?

dnsl48 commented 5 years ago

Yes, I agree, that seems a little bit strange to me too. However, .htaccess generation is happening not on dev/build, but rather on ?flush in the assets module. The error pages are rather the errorpage module responsibility. And as discussed above, it does that on dev/build, not flush. I assume we could actually make the errorpage module to repeat the approach and perform the error pages update on ?flush. Either way, we'll have to decouple that from dev/build into a reusable thing that could be done on either its own dev/... task, or on ?flush (depending on a project configuration).

albanyacademy commented 5 years ago

My mistake. Appreciate the followup.