Allow addScript to add routed scripts

[ringmaster]

Util::addScript() can be used to add scripts only if the script exists as a file.

When creating a new route to a dynamically generated javascript file from an app, Util::addScript() can be used to queue the script for output, but because the template class checks for the existence of the script as a file, and the script is virtual due to the nature of it being routed, the script file isn't found and the template class errors out.

These commits also check the router for a URL that matches the file added via Util::addScript() after the existing checks to see if the file exists in the filesystem. It allows apps to use Util::addScript() to add a script to the output that is generated by PHP via the router.

---

in app's routes.php:

/** @var OC_Route $route */
$this->create('my_app_js', '/js/custom.js')
    ->action(create_function('',"require_once __DIR__ . '/../js/custom.php';"));

in app's app.php:

OCP\Util::addScript('may_app', 'custom');

[ghost]

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/ Alternatively you can add a comment here where you state that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

[DeepDiver1975]

@owncloud-bot ok to test

[DeepDiver1975]

@ringmaster please ignore the bot

[ringmaster]

@owncloud-bot Dude, really?

@DeepDiver1975 @bantu @karlitschek @butonic Quick review, pretty please? I would like to backport this to 6.

[ghost]

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/2723/

[karlitschek]

Can't test at the moment but looks good 👍

[karlitschek]

@DeepDiver1975 @PVince81 @jancborchardt What do you think?

[MorrisJobke]

@ringmaster I tried it but it just results in an empty page. I get following error:

PHP Fatal error:  Uncaught exception 'Exception' with message ' formfactor: serverroot:/srv/http/core' in /srv/http/core/lib/private/template/resourcelocator.php:45
Stack trace:
#0 /srv/http/core/lib/private/templatelayout.php(112): OC\\Template\\ResourceLocator->find(Array)
#1 /srv/http/core/lib/private/templatelayout.php(58): OC_TemplateLayout::findJavascriptFiles(Array)
#2 /srv/http/core/lib/private/template.php(194): OC_TemplateLayout->__construct('error')
#3 /srv/http/core/lib/private/template/base.php(88): OC_Template->fetchPage()
#4 /srv/http/core/lib/private/template.php(276): OC\\Template\\Base->printPage()
#5 /srv/http/core/lib/private/template.php(308): OC_Template::printErrorPage(' formfactor: se...', '<pre>#0 /srv/ht...')
#6 [internal function]: OC_Template::printExceptionErrorPage(Object(Exception))
#7 {main}
  thrown in /srv/http/core/lib/private/template/resourcelocator.php on line 45

[ringmaster]

I'm not sure what you tried, but I think there's something off with the code you used that caused the error (maybe typos in my sample code?). Here is minimally working code that uses the change. It adds a route to a javascript file, the javascript file is generated by PHP to display the server time and script URL in an alert box every time OC loads a page.

I pushed a change (f8fbdff) that should ensure the script is relative to the correct URL when added to the output, but that shouldn't affect your issue.

[scrutinizer-notifier]

The inspection completed: 2 added/modified code elements

[icewind1991]

Since we're working on killing routed scripts and styles should this be closed?

[ringmaster]

No. This PR is required for apps to insert dynamically generated javascript of any kind.

I am unaware of any undertaking to kill routed scripts. This would be foolish, in my opinion, since, unlike how CSS references external files, there are some tasks that are impossible to accomplish otherwise.

[icewind1991]

@DeepDiver1975 opinions?

[icewind1991]

I am unaware of any undertaking to kill routed scripts.

#7310

[ringmaster]

Right. That's a totally different effort. The purpose of that PR is to serve static scripts and CSS statically (and use the server's expiry header settings for caching) instead of having to execute PHP to minimize and compress many javascript/CSS files on every request and manage the expiry of those files separately.

This PR is about allowing apps to add header references to javascript files that are meant to be generated dynamically on the server every time.

The contrast is that #7310 is about serving static files statically instead of dynamically via the front controller, and this PR is about referencing dynamically generated javascript files at all, which, without this PR, ownCloud can't do.

[MorrisJobke]

@ringmaster I tried it again, and it just stops working as soon as I enable the app, because core/js/js.js couldn't be loaded. But I don't investigate further yet. Maybe it's also a problem of my instance.

[ringmaster]

@kabum I hate to say I hope it's a problem with your instance, but I hope it's a problem of your instance. :)

I've got core/js/js.js here and it loads it fine, so I'm not sure what's going on for you.

[MorrisJobke]

@ringmaster I also hope this often :D

[DeepDiver1975]

@ringmaster you need this to inject some custom messages in the login page if I recall correctly - right?

I'd prefer to enhance the login page to pass additional template variables to the login template.

[ringmaster]

@DeepDiver1975 There are many small issues with that. First, the output of the additional template variable needs to be made possible. There are places, for example, where an error can be output, but these places are styled as errors and include static content in the template that would be inappropriate for my purposes. A separate location for output would need to be created, and would have to accommodate any output style, since the "message" I need to inject may also at some times include a separate form which may need to be styled differently than other messages. This styling would have to be included in the variable output. I don't think this is ideal to include as a variable.

Regardless of whether including styling markup in the variable output is ideal, there is no way I could find to assign a template variable from an app such that it would be included on the login page. So even if there was a place to output a new variable, there's no way to get the data into the template for that purpose.

The better way to accomplish all of this would be to include hooks for template rendering, such that before a template is rendered, the template variables could be altered/augmented, or the template itself changed prior to output. This is a more comprehensive and general solution, and I thought this would be better to include in a full version number release with some other hook handling changes.

That is all to say, I agree that I would prefer to generically inject code into the output that way, but this change seems too big for a point release. If you're suggesting that I can get a faster/easier approval for backport to 6.0.2 on that more comprehensive change, then I'll work on that immediately instead. I expect this change for OC7 (even if I write it myself), regardless.

That said, the addScript() method is attractive to me because it does two things:

1. It accomplishes the task I need to solve.
2. It fixes a well-defined problem where addScript() will not accept the URL of a valid route as a script to add. This is a general failing of the addScript() code, since it will currently only accept URLs that point to static scripts, and not to URLs that will generate output dynamically.

[ringmaster]

How would you feel about a hook in core/js/config.php instead that allowed me to modify the $array array?

[ringmaster]

Ok, this code doesn't seem to work for anyone but me, and nobody seems to like the idea anyway, so I'm going try a new PR to modify the $array array in config.php via hook instead.

Thanks for checking it out.