silverstripe / cwp-pdfexport

Adds PDF export capability for CWP pages
BSD 3-Clause "New" or "Revised" License
4 stars 5 forks source link

$generated_pdf_path points to old asset path #8

Closed torleif closed 5 years ago

torleif commented 5 years ago

If you generate a PDF file from a webpage, it gets saved to assets/_generated_pdfs. However with a SilverStripe 4.3 install the default asset path abstracted like this: public/assets/[hashoffile]/filename.fileext

Abstracting the file into the asset management system would cause more problems than what it's worth.

I'd say a new dedicated folder inside public/ would perhaps make the most sense, such as public/pdfs/ or public/_generated_pdfs/

robbieaverill commented 5 years ago

Abstracting the file into the asset management system would cause more problems than what it's worth.

I think you're probably right, given the context is CWP and assets are almost always going to be on the physical filesystem.

I think the fix here is to make the assets/_generated_pfds folder prepend public/ when configured. For CWP 2.x it's mandated by the way, so even though we don't need to we can enforce it by hard coding it.

The logic should be something like this:

        if ($folderPath[0] != '/') {
-             $folderPath = BASE_PATH . '/' . $folderPath;
+             $folderPath = Director::publicFolder() . '/' . $folderPath;
        }

While we're at it, assets/ is also configurable, so we should build in some support for configuring that e.g. a placeholder like private static $generated_pdf_path = '{assetsDir}/_generated_pdfs'; and str_replace('{assetsDir}', ASSETS_DIR, $folderPath)

robbieaverill commented 5 years ago

Confirmed. It's putting it in the root project instead of the correct assets dir.

robbieaverill commented 5 years ago

PR at https://github.com/silverstripe/cwp-pdfexport/pull/9