Feature/script tag

[robsonsobral]

Overview

It implements:

• {script="group/template"}
• {stylesheet="site:group/template"}

Resolves #1246.
Addresses #1220.

Nature of This Change

• 🐛 Fixes a bug
• 🚀 Implements a new feature
• 🛁 Refactors existing code
• 💅 Fixes coding style
• ✅ Adds tests
• 👽 Adds new dependency
• 🔥 Removes unused files / code
• 🔒 Improves security

Is this backwards compatible?

• Yes
• No

Documentation

User Guide Pull Request: ExpressionEngine/ExpressionEngine-User-Guide#232

[robsonsobral]

As use the Stylesheet library to serve JavaScript feels weird, I duplicated and renamed it to Assets. I guess, the original can be converted into a proxy to the new one, to keep it available to addons developers. However, I've no idea on the better path to take.

[intoeetive]

I don't like duplicatig the code - I think we need to be as DRY as possible.

I think it's better to have something like class Javascript extends Stylesheet - however we already have Javascript library so this might bring extra confusion.

Maybe this would be good chance to take this code out of legacy folder and make it Library or Service. And then the legacy class would just inherit the new one

[robsonsobral]

@intoeetive , do you mind help me a little more with the right places and naming conventions? Should I create an Assets Library? Is that fine?

[intoeetive]

I think making this change is combination of adding new feature with code refactoring.

We generally try to avoid adding stuff into legacy folder (we still do that for fixes though or when full component rewrite would be required otherwise). I also don't like the name Assets (that's just a personal thing; I think it brings confusion with the image management add-on), I prefer something like Resources.

So if I would be you :) I'd probably do something similar to this:

1. Under ExpressionEngine\Libraries create Resources/Stylesheet and Resources/Javascript libraries, and have those registered in app.setup.php
2. I would also probably create something like Resources/AbstractResource.php that the libraries could inherit or partially reuse. I'd put there stuff like sending generic headers, or adding hashsum etc.
3. Keep the legacy EE_Stylesheet, but replace all functions code will calls to new library. Make it log deprecation message when called
4. Update EE code to use the new library
5. Make the other necessary changes to implement the new feature (in Template library etc)

Again, this is just how I personally view things here. I admit it can be approached differently as well

[robsonsobral]

I prefer to follow your lead, as I'm a JS Developer and you'll be taking care of this code in the long run.

This way?

Here we go!

[intoeetive]

Yes, something along those lines should be fine

(please also try to avoid force-pushing, that makes merging more difficult)

[robsonsobral]

I'm sorry it have been taking so long, @intoeetive . Not even being an important feature for me, I've not found time enough for.

While working, I've found a bug which should not have to wait for me to be fixed! The Last-Modified header submitted along CSS requests are wrong when using template files!

/** -----------------------------------------
/**  Retrieve template file if necessary
/** -----------------------------------------*/
if (ee()->config->item('save_tmpl_files') == 'y') {
    ee()->load->helper('file');
    $basepath = PATH_TMPL . (
        isset($site) ? $site : (ee()->config->item('template_folder') ?: ee()->config->item('site_short_name'))
    ) . '/';
    $basepath .= $group . '.group/' . $row['template_name'] . '.' . $this->type;

    $str = read_file($basepath);
    $row['template_data'] = ($str !== false) ? $str : $row['template_data'];
    $row['edit_date'] = ($str !== false) ? filemtime($basepath) : $row['edit_date']; // this line is missing!
}

Without that last line, the submitted date is from database, not saved files.

[robsonsobral]

I'm sorry, @intoeetive . I proved myself a total newbie PHP developer. I've saved my files in the right places, but I couldn't make them to work.

[intoeetive]

I'll try to go with it. Can't promise when that will be happening though

[robsonsobral]

I'm sorry, @intoeetive . And thank you!

[robsonsobral]

I did it, @intoeetive ! It worked!

[robsonsobral]

@intoeetive , 😥😥😥?

[intoeetive]

I looked through the code, and I think you did awesome job here.

I did not check every tiny bit, but it does look very good to me.

However we need to have some automated tests written to accept this - do you happen to have sample template code, or add-on code, that's utilizing this new code and that we can use to test?

Also in few places there need to be spaces around . to make the code PSR-12 compatible.

Thank you very much!

[robsonsobral]

Tests added!

The same tests can be used to test the integrity attribute in the future.

[robsonsobral]

I guess I should add tests for trigger words on URLs, like https://example.com/css/channel/channel_css/.

[robsonsobral]

Now I've a problem. To add js as a template trigger word is a breaking change. May I ignore this? I never used this feature, so I don't know how important it is.

[intoeetive]

Adding js as reserved word might be a problem if the system already has template or group named js. So need to think about how to handle this best - maybe have this as configurable value

[robsonsobral]

Options I have:

1. to not add js as a template trigger word. I don't know how important trigger words are. For example, xml is a valid template type which doesn't have trigger word on URI. Also, there's an annoying dev suggesting adding json as a valid template. Does it mean it requires a trigger word as well? Who knows?
2. to put {script} tag behind a configuration. Which one?
3. to put js as a trigger word behind a configuration. Which one?;
4. to delay {script} tag until eecms v7 (and make me cry for my life).

Options 1 and 3 would make template type trigger words to be considered almost as a to-be-deprecated feature.

In any case, /css/ needs to be added to tests and I'm going to do that.

[intoeetive]

I've never used the "trigger word" feature myself, so don't know how important it is. My vote is option 1.

[intoeetive]

I have fixed the tests. Letting you guys handle everything else!