clubstudio / craft-asset-rev

A Craft CMS plugin to help with cache busting
MIT License
109 stars 7 forks source link

`assetsBasePath` no longer works in v4.x #6

Closed joshangell closed 7 years ago

joshangell commented 7 years ago

As far as I can tell assetsBasePath no longer works since v4. If I downgrade to v3.0.0 it works just fine. I am using the following configuration options:

assetrev.php

<?php
return array(
    '*' => array(
        'manifestPath' => '../public/rev-manifest.json',
        'assetsBasePath' => '{cdnUrl}assets/',
    ),
);

general.php

<?php
return array(
    'environmentVariables' => array(
        'cdnUrl' => 'http://craft-cdn.dev/'
    ),
);

In v3 it outputs something like http://craft-cdn.dev/assets/css/style.min.css whilst in v4 I get css/style.min.css.

Brilliant plugin though, its in our boilerplate 😄

scottwakefield commented 7 years ago

Hi Josh,

Thanks for the report!

With V4 there are two 'prefix' settings; one is the assetBasePath and the other (new) one is assetUrlPrefix. We needed to use assetBasePath as the local file path because V4 supports fallback to query string, which needs to be able to find the actual source file in order to generate the query string.

I've just noticed that the readme doesn't mention assetUrlPrefix in the config example, so for people upgrading I can see why it would be confusing.

Here's the contents of the new default config:

<?php
return array(
    'manifestPath' => 'resources/assets/assets.json',
    'assetsBasePath' => null,
    'assetUrlPrefix' => null,
);

Try setting the assetUrlPrefix to {cdnUrl}assets/ and see if that works. If not, come back to me and I'll look into it!

Thanks again.

joshangell commented 7 years ago

It works! I did try that initially, but I now realise that was with v4.0.0 and not v4.0.1, which fixes that :roll_eyes:

Stand down!

joshangell commented 7 years ago

Without wanting to open another issue, in v3 a nice side-effect of setting manifestPath to something invalid (like '.' or whatever) was that it would stop the plugin from finding a file and just fall back to outputting whatever was inside {{ rev(...) }}.

We used to use that to ignore revved files locally but still use them in production, is there perhaps a way we can have a setting added to allow us to do similar?

scottwakefield commented 7 years ago

Ah ok. So, what happens in V4 if you just pass through . as the manifestPath? Is it trying to find the non-revved file and the throwing an exception because it can't?

Are you wanting to totally ignore revved files? So that they essentially cause a 404 while you're developing?

I'm open to adding an option for this once I understand the need properly :)

joshangell commented 7 years ago

Yes exactly that, I get an InvalidArgumentException thrown:

Cannot append query string - the file css/style.min.css does not exist.

What I would like is to be able to ignore the revved file and just output the source file for my local environment. Then I when I’m running my gulp watch tasks on my local I don’t have to build out revisioned versions of the files. It just speeds things up a bit.

I’m imagining the other settings would still work though, so it can’t find the manifest and therefore just uses the source file I put in the template code, after that it still prepends whatever is in assetUrlPrefix to the end result.

Does that make any sense?!

scottwakefield commented 7 years ago

Yep, I'm following you.

So when you're running gulp watch locally it's not compiling to style.min.css and then in production to style.min.12345.css?

The workflow we use is:

style.css and style.12345.css never exist at the same time.

So, in the template we're writing {{ rev('style.css') }} and that appends a query string for local dev when it can't find the manifest file.

Is your workflow different? I suspect it is.

👍

joshangell commented 7 years ago

Aha, yes ours is different (for legacy reasons ...) and it leaves style.css, style.min.css and style-12345.min.css all in place. I actually forget why that is, and I no longer manage the front end build stuff so actually have no idea if its still relevant ... I just know that as a result of having all 3 still loitering around its nice to be able to just switch off the revved files!

So, in short perhaps we should look at our workflow, but also perhaps there are other more valid reasons why someone might need to turn it off for a given environment.

scottwakefield commented 7 years ago

Thanks for the extra info Josh! I'll take a look at implementing something ASAP.

joshangell commented 7 years ago

No worries, there’s no rush from our end 🙂 I know how it goes - client work takes priority!

cmalven commented 7 years ago

Similar issue here. I think our workflow is similar to yours @scottwakefield but for me, when the rev-manifest.json can't be found, rev just blows up with Cannot append query string - the file/dist/scripts/main.jsdoes not exist. despite the fact that the file in question definitely does exist.

Should this be happening? I got the sense from your comment that if rev-manifest.json can't be found it should just use the not-revved asset.

scottwakefield commented 7 years ago

Hi @cmalven!

Sorry to hear that you're having trouble. The usual cause for the Cannot append query string... issue is that the path to your assets isn't set correctly.

First thing to check is that assetsBasePath is set correctly in your config file. Note that the paths in the config file are relative to your /craft/ directory.

Because the latest version falls back to adding a query string (that's generated from the files last modified date), the plugin needs to know the path to your built asset files.

If you still have no joy, provide your config file and directory structure and I'll have a look into it! :)

cmalven commented 7 years ago

Thanks for the super prompt response @scottwakefield!

Indeed, setting an assetBasePath did the trick. We've used this plugin on almost every site for awhile now, but we were always revving the assets during development as well and so never ran into this.

Thanks again, and thanks for the great plugin!

scottwakefield commented 7 years ago

No problem @cmalven. Glad to hear that did the trick and that you've been finding the plugin useful!

If there's anything else I can help with just give me a shout.