Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-9978

Missing semicolons in // <![CDATA[ part of overall_header.html

    Details

    • Type: Bug
    • Status: Unverified Fix
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: 3.0.8
    • Fix Version/s: 3.0.10-RC1
    • Component/s: Styles
    • Labels:
      None
    • Environment:
      phpBB 3.0.8, MySQL 5.1.34, PHP Version 5.2.14, FireFox 3.6.13, CSE HTML Validator v10.01

      Description

      Trying to track down another issue I stumbled across this. The following is extracted from rendered code. Source seems to be from Overall_header.html

      validator returns: [JavaScript] lint warning: missing semicolon
      for the beginning of the function: window.onunload = function()
      Seems to be related to the two new functions onload and onunload

      <script type="text/javascript">
      // <![CDATA[
      	var jump_page = 'Enter the page number you wish to go to:';
      	var on_page = '1';
      	var per_page = '';
      	var base_url = '';
      	var style_cookie = 'phpBBstyle';
      	var style_cookie_settings = '; path=/; domain=www2.knob.com';
      	var onload_functions = new Array();
      	var onunload_functions = new Array();
       
      	
       
      	/**
      	* Find a member
      	*/
      	function find_username(url)
      	{
      		popup(url, 760, 570, '_usersearch');
      		return false;
      	}
       
      	/**
      	* New function for handling multiple calls to window.onload and window.unload by pentapenguin
      	*/
      	window.onload = function()
      	{
      		for (var i = 0; i < onload_functions.length; i++)
      		{
      			eval(onload_functions[i]);
      		}
      	}
       
      	window.onunload = function()
      	{
      		for (var i = 0; i < onunload_functions.length; i++)
      		{
      			eval(onunload_functions[i]);
      		}
      	}
       
      // ]]>
      </script>
      

        Activity

        Hide
        callum95 callum95 added a comment -

        This is just JSLint complaining that we don't use semi-colons on the end of functions. It's nothing to worry about, the code is perfectly valid, it just isn't "clean" - at least, not clean how JSLint defines clean to be.

        Show
        callum95 callum95 added a comment - This is just JSLint complaining that we don't use semi-colons on the end of functions. It's nothing to worry about, the code is perfectly valid, it just isn't "clean" - at least, not clean how JSLint defines clean to be.
        Hide
        Oleg Oleg [X] (Inactive) added a comment -

        The syntax is technically valid, however we will merge a pull request adding the specified semicolons

        Show
        Oleg Oleg [X] (Inactive) added a comment - The syntax is technically valid, however we will merge a pull request adding the specified semicolons
        Hide
        callum95 callum95 added a comment - - edited

        Okay, it isn't just the semi-colons. I personally dislike the JSLint syntax. This passes:

        var jump_page = '{LA_JUMP_PAGE}:';
        var on_page = '{ON_PAGE}';
        var per_page = '{PER_PAGE}';
        var base_url = '{A_BASE_URL}';
        var style_cookie = 'phpBBstyle';
        var style_cookie_settings = '{A_COOKIE_SETTINGS}';
        var onload_functions = [];
        var onunload_functions = [];
         
        var url = '{UA_POPUP_PM}';
        window.open(url.replace(/&amp;/g, '&'), '_phpbbprivmsg', 'height=225,resizable=yes,scrollbars=yes, width=400');
         
        /**
        * Find a member
        */
        function find_username(url) {
        	"use strict";
        	popup(url, 760, 570, '_usersearch');
        	return false;
        }
         
        /**
        * New function for handling multiple calls to window.onload and window.unload by pentapenguin
        */
        window.onload = function () {
        	"use strict";
        	var i;
        	for (i = 0; i < onload_functions.length; i += 1) {
        		onload_functions[i]();
        	}
        };
         
        window.onunload = function () {
        	"use strict";
        	var i;
        	for (i = 0; i < onunload_functions.length; i += 1) {
        		onunload_functions[i]();
        	}
        };

        It doesn't allow inline javascript, either.

        Show
        callum95 callum95 added a comment - - edited Okay, it isn't just the semi-colons. I personally dislike the JSLint syntax. This passes: var jump_page = '{LA_JUMP_PAGE}:'; var on_page = '{ON_PAGE}'; var per_page = '{PER_PAGE}'; var base_url = '{A_BASE_URL}'; var style_cookie = 'phpBBstyle'; var style_cookie_settings = '{A_COOKIE_SETTINGS}'; var onload_functions = []; var onunload_functions = [];   var url = '{UA_POPUP_PM}'; window.open(url.replace(/&amp;/g, '&'), '_phpbbprivmsg', 'height=225,resizable=yes,scrollbars=yes, width=400');   /** * Find a member */ function find_username(url) { "use strict"; popup(url, 760, 570, '_usersearch'); return false; }   /** * New function for handling multiple calls to window.onload and window.unload by pentapenguin */ window.onload = function () { "use strict"; var i; for (i = 0; i < onload_functions.length; i += 1) { onload_functions[i](); } };   window.onunload = function () { "use strict"; var i; for (i = 0; i < onunload_functions.length; i += 1) { onunload_functions[i](); } }; It doesn't allow inline javascript, either.
        Hide
        Oleg Oleg [X] (Inactive) added a comment -

        The other complaints do look a little excessive to me. I would say just add the two semicolons.

        The reason to have semicolons in standalone javascript files is to aid minification by "dumb" minifiers that do simple replacements. Since inline js won't be minified this is kind of a moot point, however, inline js can be moved into a file to become standalone js at which point lack of semicolons would become somewhat important.

        I can agree with replacing new Array() with [], but increment and decrement operators really are just fine. And our indentation is correct. No need for use strict either. Putting var on top is not needed if we are adults and know what we are doing. There is actually an opposite convention in C++ which says that variable declarations should be as close as possible to their usage.

        Show
        Oleg Oleg [X] (Inactive) added a comment - The other complaints do look a little excessive to me. I would say just add the two semicolons. The reason to have semicolons in standalone javascript files is to aid minification by "dumb" minifiers that do simple replacements. Since inline js won't be minified this is kind of a moot point, however, inline js can be moved into a file to become standalone js at which point lack of semicolons would become somewhat important. I can agree with replacing new Array() with [], but increment and decrement operators really are just fine. And our indentation is correct. No need for use strict either. Putting var on top is not needed if we are adults and know what we are doing. There is actually an opposite convention in C++ which says that variable declarations should be as close as possible to their usage.
        Hide
        callum95 callum95 added a comment -

        Okay, I'll send a pull request in a bit. 3.1 or 3.0?

        Show
        callum95 callum95 added a comment - Okay, I'll send a pull request in a bit. 3.1 or 3.0?
        Hide
        Ag2000CO Ag2000CO added a comment -

        Oleg I agree - two semicolons. The rest IMHO have to do with style and readability, nice but more about opinion than standards compliance. Thanks for lots of great work.

        Show
        Ag2000CO Ag2000CO added a comment - Oleg I agree - two semicolons. The rest IMHO have to do with style and readability, nice but more about opinion than standards compliance. Thanks for lots of great work.

          People

          • Assignee:
            Oleg Oleg [X] (Inactive)
            Reporter:
            Ag2000CO Ag2000CO
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development