Javascript escape characters showing

[Schmoopy]

Well I don't know why, but doing what you say still comes back with \' \" etc, even if I try htmlspecialchars().

I changed it to the following and it's gotten a little better, but still breaks if I try to put + or & in the string:

Javascript which passes values to PHP:

function editField(id, field)
{
	var defaultVal = document.getElementById(field).innerHTML;
	var inputval = prompt('Please enter a new value',defaultVal);
	if(inputval != "" && inputval != null && inputval != defaultVal) // Check if empty, cancelled or value not changed
	{	
		new Ajax.Request("editjob.php", 
		{ 
		method: 'post', 
		postBody: 'id=' + id + '&val=' + inputval + '&field=' + field,
		onComplete: refreshField
		});
	}
}

$val = mysql_real_escape_string($_POST['val']);
....
$query = "UPDATE enquiries SET `$databasefield` = '$val' WHERE `id` = '$id'";
...
echo $type . $_POST['val']; // The type is there for the AJAX to strip off 

Goes to javascript function (refreshTable):

var type = req.responseText.substring(0,1);
	var fulltext = req.responseText.substring(1);	
	
	switch(type)
	{
	case '1':
		field = 'customername';
		break;
	case '2':
		field = 'customeraddress';
		break;
	case '3':
		field = 'customerphone';
		break;
	case '4':
		field = 'customerfinish';
		break;
	case '5':
		field = 'customeremail';
		break;
	}
	document.getElementById(field).innerHTML= fulltext;

It takes the type, then according to what it is, it displays it in different fields, then outputs the fulltext, which is the response - the type.

I know this is probably terrible but when it comes to javascript, let alone using prototype and ajax it's the best I could do.

[Twey]

Well, of course: you've jammed everything together into a string, which means you have to escape the parameters. + and & are both special in HTTP URL syntax. The appropriate escape function is encodeURIComponent(), but easier would just to be to avoid passing things around as strings in the first place:

function editField(id, field) {
    var defaultVal = document.getElementById(field).innerHTML,
        inputVal   = prompt("Please enter a new value",
                            defaultVal);

    if (inputVal && inputVal != defaultVal) // Check if empty, cancelled or value not changed
        new Ajax.Request("editjob.php", {
                             onComplete: refreshField,
                             method: 'post',
                             parameters: {
                                 id: id,
                                 val: inputVal,
                                 field: field
                             }
                         });
}

It will presumably work after that, but it can still be improved in several ways. The first is the use of innerHTML. innerHTML is non-standard and should be avoided where possible. In this case, it looks like you're actually dealing with a form element. If you are, you can use .value instead:

function editField(id, field) {
    var f        = document.getElementById(field),
        inputVal = prompt("Please enter a new value", f.value);

    if (inputVal && inputVal != f.defaultValue) // Check if empty, cancelled or value not changed
        new Ajax.Request("editjob.php", {
                             onComplete: refreshField,
                             method: 'post',
                             parameters: {
                                 id: id,
                                 val: inputVal,
                                 field: field
                             }
                         });
}

The second is that big clumsy switch statement. Switch statements are rarely useful in Javascript: usually we can use arrays or objects instead:

var f = document.getElementById(["customername",
                                 "customeraddress",
                                 "customerphone",
                                 "customerfinish",
                                 "customeremail"][req.responseText.charAt(0) - 1]);

(f.firstChild || f.appendChild(document.createTextNode("")))
 .nodeValue = req.responseText.substring(1);

Note that again I've removed the innerHTML usage there; if you really did need to send a big chunk of HTML over the network, you should rethink your design.

If you're just starting out with Javascript, I really don't recommend Prototype.js to get you started. It is exemplary, but only, I fear, as a warning of how not to code.

Additionally, we don't really use this style very much in Javascript:

controlKeyword(foo)
{
    bar;
}

This is because

{
    bar;
}

is a block, and it is perfectly valid standing on its own. The only way we can visually distinguish that style from a function call followed by a block is by checking for a semicolon at the end of the control statement, and since semicolons are optional in Javascript, even that isn't reliable. Rather, we both separate the keyword from the following brackets with a space and put the opening brace on the same line as the control flow statement:

controlKeyword (foo) {
    bar;
}

We also indent the contents of the block, so we know at which level the contents belong (I noticed you didn't do that with your switch statement). Of course, if you have only one statement affected by the control flow statement, you needn't bother with the braces at all: you can simply indent it properly and make it much clearer to what each statement belongs (since it always belongs to the control flow statement immediately above it).

Fear not: there is no AJAX in the code you provided above (you're using a simple text format, not XML).

My recommendation to use PDO on the PHP side of things stands.

[Schmoopy]

Thanks for that, really helpful and has fixed the problem pretty much, it's just that when I go to edit the field the value in the prompt box says "undefined" instead of the value of the field. I know it's bad practice to send variables in strings and to control the flow like that but it's the only way I could figure out by doing it myself.

Another thing, could you explain this part of the code? I don't understand what it's doing:

var f = document.getElementById(["customername",
                                 "customeraddress",
                                 "customerphone",
                                 "customerfinish",
                                 "customeremail"][req.responseText.charAt(0) - 1]);

(f.firstChild || f.appendChild(document.createTextNode("")))
 .nodeValue = req.responseText.substring(1);

[Twey]

Thanks for that, really helpful and has fixed the problem pretty much, it's just that when I go to edit the field the value in the prompt box says "undefined" instead of the value of the field.

If you used the code formulated for a form element on a non-form element (i.e. one without a value), then that will happen, yes.

I know it's bad practice to send variables in strings and to control the flow like that but it's the only way I could figure out by doing it myself.

Interesting; you managed to figure out the most convoluted and difficult way of doing it without any bad influences?

Another thing, could you explain this part of the code? I don't understand what it's doing:

First, we select the element, getting its ID from the array using the ‘type’ (the first character of req.responseText) as an index, and save it to the variable f; then, we set the nodeValue of the first child of f (or a new text node appended to f, if no appropriate node exists) to the rest of req.responseText.

[Schmoopy]

O.K, I still don't fully understand - [req.responseText.charAt(0) - 1], this to me looks like:

Get first character in string (0), "Hello" -> "H", but then - 1? Don't understand that.

Edit: Ohh, type is an integer - I forgot, so the type, say 1 - 1, will be 0 so it can go to customername for example. Me being stupid

(f.firstChild || f.appendChild(document.createTextNode("")))
.nodeValue = req.responseText.substring(1);

Any simpler explanation for the above? Hehe, I'm just really not used to nodes.

If you used the code formulated for a form element on a non-form element (i.e. one without a value), then that will happen, yes.

To fix this, I reverted back to f.innerHTML, is this a good move or not? Since it's not a form element is there any "better" way to retrieve the value?

[Twey]

Yes: store it separately from the HTML, so you don't have to go through the trouble of extracting it again.

Any simpler explanation for the above? Hehe, I'm just really not used to nodes.

If f has a firstChild, use that; otherwise, append a new empty text node and use that. Either way, set the element's nodeValue to everything but the first character from req.responseText.

[Schmoopy]

Well, the value is stored separately from the HTML, since it echos from the database, but I don't see what you mean, since javascript can't echo data from a database, what other way of storing it would be possible?

[Twey]

What you're doing now, I presume, is echoing the value from PHP into the HTML, then reading the HTML with Javascript. Instead, try echoing it straight into Javascript — that way, no extraction is necessary.

[Schmoopy]

O.K, by that I presumed you meant something like this:

function editField(id, field, normalValue)
{
	var f = document.getElementById(field),
	inputVal = prompt('Please enter a new value',normalValue);
	if(inputVal && inputVal != normalValue) // Check if empty, cancelled or value not changed

And then pass the row from PHP into the function:

onclick=\"editField('{$id}','customername','{$row['name']}')\"> 

Edit: Having problems with the above method, lol . If I put an apostrophe it terminates the string so I'm guessing this probably isn't the way to do it.

One more thing:

How do I escape a "£" sign in an alert? I've tried &pound; but it just prints it out literally, I searched around on google but everyone thinks a pound sign is "#" >.<

[Twey]

That should be more or less fine, although from that snippet it looks like you're echoing markup from PHP. Don't do that — break out of PHP parsing mode. It's faster, neater, and easier.