remove <script> from page_table.inc

[70ray]

contribution to issue #1298 Sandbox at: https://www.pgdp.org/~rp31/c.branch/unscript_project

[70ray]

Sandbox updated.

[70ray]

I think this PR didn't update this call to echo_page_table that would need to include the new JS file, no?

I am slightly concerned about the hidden assumption that calling echo_page_table now requires to import the script in the caller or else the selection will not work. It would probably be better to have it output a <script defer src="..."> inside echo_page_table but I could be missing something.

I'm not sure if I have understood your concern correctly but do_page_table() is only called in project.php so the js file will be loaded in the page head by output_html_header() in html_page_common.inc. If I understand it correctly, issue #1298 requires removing all embedded js from php files, not because the js is bad in itself but because it makes it easier to enhance security by eventually disallowing it.

[70ray]

Sandbox updated.

[jchaffraix]

First, sorry if my previous comment was unclear. Also I am still fairly new to DP development so feel free to point out if this doesn't make sense.

I think this PR didn't update this call to echo_page_table that would need to include the new JS file, no? I am slightly concerned about the hidden assumption that calling echo_page_table now requires to import the script in the caller or else the selection will not work. It would probably be better to have it output a <script defer src="..."> inside echo_page_table but I could be missing something.

I'm not sure if I have understood your concern correctly but do_page_table() is only called in project.php so the js file will be loaded in the page head by output_html_header() in html_page_common.inc. If I understand it correctly, issue #1298 requires removing all embedded js from php files, not because the js is bad in itself but because it makes it easier to enhance security by eventually disallowing it.

The concern is that the code prior to this PR would automatically register an onchange listener for the table generated by echo_page_table (on this line). The new code puts this logic into a separate JS file that needs to be explicitly loaded. This means that a caller needs to be aware that any calls to echo_page_table down the stack requires some special handling. The suggestion was to let echo_page_table automatically generate a deferred <script> tag that would pull this needed dependency.

[cpeel]

The concern is that the code prior to this PR would automatically register an onchange listener for the table generated by echo_page_table (on this line). The new code puts this logic into a separate JS file that needs to be explicitly loaded. This means that a caller needs to be aware that any calls to echo_page_table down the stack requires some special handling. The suggestion was to let echo_page_table automatically generate a deferred <script> tag that would pull this needed dependency.

Your comment makes sense to me and this two-step process does introduce the possibility of an error. I think in this case its worth it, as having the JS file included as part of our header code allows the introduction of a nonce for these in one place rather than spread around the code. What would probably be ideal is for an .inc file to "register" the need for a JS file if it is included such that the JS is automatically pulled in (or just bundle all of our JS together somehow into a single file). I think that's overkill for this situation though.