Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

So many times on this site I see people trying to do things like this :

<script type="text/javascript">
  $(document).ready(function(){

     $('<?php echo $divID ?>').click(funtion(){
       alert('do something');
     });

  });
</script>

I don't think that this is some sort of pattern that people naturally fall into. There must be some sort of tutorial or learning material out there that is showing this, otherwise we wouldn't see it so much. What I'm asking is, am I making too big a deal of this or is this a really bad practice?

EDIT : Was speaking to a friend of mine about this who often puts ruby in his JavaScript and he brought up this point.

Is it ok to dynamically place application wide constants in your JavaScript so you don't have to edit two files. for example...

MYAPP.constants = <php echo json_encode($constants) ?>;

also is it OK to directly encode data you plan to use in a library

ChartLibrary.datapoints = <php echo json_encode($chartData) ?>;   

or should we make an AJAX call every time?

share|improve this question

migrated from stackoverflow.com Dec 23 '11 at 0:28

This question came from our site for professional and enthusiast programmers.

4  
It seems to me that this question will likely solicit opinion, debate, arguments, polling, or extended discussion.... –  DaveRandom Dec 22 '11 at 16:49
6  
@M.Babcock This would be part of a .php file, so that php code would get executed server-side, and the client would only see the result of the echo. –  thatwasbrilliant Dec 22 '11 at 16:51
7  
Anyone who creates dynamically generated JavaScript get's taken out back and is dealt with –  Raynos Dec 22 '11 at 16:55
5  
@Matt then I'll take google out back and deal with them –  Raynos Dec 22 '11 at 17:00
3  
Yes, this is bad, it shows you fail at understanding what an ID tag is, and your semantic HTML is likely to be a pile of trash. –  Incognito Dec 22 '11 at 17:00
show 12 more comments

13 Answers

up vote 61 down vote accepted

Typically, it is bad practice to use language X to generate code in language Y.

Try decoupling the two languages by making data their only interface -- don't mingle the code.

In your example, you could improve the code by using PHP to populate a cfg structure that's available to JavaScript:

<script type="text/javascript">
  var cfg = {
    theId: "<?php echo $divID ?>",
    ...
  };

  $(document).ready(function(){
     $("#" + cfg.theId).click(funtion(){
       alert('do something');
     });
  });
</script>

This way, PHP only cares about populating the data structure and JavaScript only cares about consuming the data structure.

This decoupling also leads the way to loading the data asynchronously (JSON) in the future.

Update:

To answer the additional questions you asked with your update, yes, it would be good practice to apply the DRY principle and let PHP and JavaScript share the same configuration object:

<script type="text/javascript">
  var cfg = <?php echo json_encode($cfg) ?>;

  ...

There is no harm in inserting the JSON representation of your configuration directly into your page like this. You don't necessarily have to fetch it via XHR.

share|improve this answer
16  
Remember not to put your MySQL password in $cfg though!! –  Andreas Bonini Dec 22 '11 at 20:02
6  
"making data their only interface -- don't mingle the code." I think this really cuts to the core of the issue and is a good rule of thumb when using ANY two languages together. Thanks for the insight. –  Greg Guida Dec 22 '11 at 20:39
4  
You could also include that JSON in a data- attribute in your HTML. Something like <body data-cfg="{...}">. –  kapa Dec 22 '11 at 21:27
1  
@bazmegakapa I think that might be the best option. In particular, it allows the use of APIs such as HTML DOM that greatly reduce the risk of XSS injections. –  luiscubal Dec 22 '11 at 23:00
    
+1 for suggesting using data as the interface and discouraging code that generates code. –  Brandon Mar 3 '13 at 0:04
show 1 more comment

Dynamically generated JavaScript is a horrible, bad practice.

What you're supposed to do is comprehend what Seperation of Concerns and Progressive Enhancement mean.

This basically means you have dynamic HTML and static JavaScript (which enhances the HTML).

In your case you probably want a class on your div and select it with a class selector

share|improve this answer
add comment

The biggest problem with your snippet is you're missing the # to make it a valid jQuery selector ;).

I would say you should try avoid including PHP in your JavaScript where possible. What's wrong with changing the selector in your click() handler to a class, and adding the class to the element in question if you want the handler to be fired, and not if you don't;

<script type="text/javascript">
  $(document).ready(function(){

     $('.foo').click(funtion(){
       alert('do something');
     });

  });
</script> 

<div id="bar" class="<?php echo ($someCond ? 'foo' : ''); ?>">Hello</div>

There are circumstances where you need to include PHP in your JavaScript; but I must admit these are few and far between.

Once example is when you have different environments; test, staging and live. Each of them have a different location of your assets (images, mainly). The easiest way to set the path so that it can be used by JavaScript is something like;

var config = { assets: "<?php echo $yourConfig['asset_url']; ?>" };
share|improve this answer
    
In the rest of my imaginary php code I've already appended the # =) but seriously I agree that you're example is the better way to do it. It seems more natural to me to do it that way too. So why do we see it so often in places where it is not necessary? –  Greg Guida Dec 22 '11 at 17:06
    
Note that echoing static data in a config file can be easily avoided by having your environments all be setup in the same way. –  Raynos Dec 22 '11 at 17:08
4  
@GregGuida: My guess would be that programmers are seldom used to dealing with client/ server architectures like you get in web development. They treat DB <-> PHP <-> HTML / JS / CSS as one, and don't fully understand what should go where, and how the layers should be separated. –  Matt Dec 22 '11 at 17:10
    
@Matt I think that that is probably the best explanation –  Greg Guida Dec 22 '11 at 17:12
2  
$divID = '#' . $element_id_value; - no problems with the selector boss ;) –  rlemon Dec 22 '11 at 17:12
show 1 more comment

I don't think this is bad practice. If the ID required in your JavaScript is dynamic there is no other way to do this.

share|improve this answer
4  
Why in the unholy name of cuthulu would you not know the name of an ID tag? –  Incognito Dec 22 '11 at 16:58
2  
@Matt Okay, so when you want to write broken semantic HTML, I get it. –  Incognito Dec 22 '11 at 17:02
2  
@Incognito, there are lots of times when you don't know the ID... If you're using ajax to generate new blocks of code you may be generating unique IDs along with new JS... in these cases you need to make sure that the IDs being references are the same in the js are they are in the resulting code block. There are lots more instances like this and it's pretty common when ajax is involved or there are large blocks of code dependent on a server-side if-else statement etc –  Ben D Dec 22 '11 at 17:10
6  
@raynjamin I don't even understand how you put yourself into situations where that's what you're doing... or why you'd select by class, then list tags, then ID attributes which have a hidden css value, that selector man... it's actually paining me to look at... I don't even know where to start with... like... what? Are you cutting+pasting massive blocks of code or something to make things work over multiple IDs? I don't even... like... my brain. it's exploding here. –  Incognito Dec 22 '11 at 21:18
3  
no. you don't understand how ID tags work. I understand exactly what you're saying. –  Incognito Dec 22 '11 at 23:18
show 12 more comments

This is a bad practice in my opinion, as you would need to call this file something.php and then you couldn't compress it for example, not mentioning it isn't ok to mix in your server stuff with your JavaScript. Try to limit the mixing between PHP and JS as much as possible.

You can always do this instead:

function setOnClick(divid) {
 $(divid).click(funtion(){
   alert('do something');
 });
}

And then you could call this function in a php file, to make these mixing things as small as possible.

$(function() {
  setOnClick('<?php echo $divId; ?>');
});

By doing this (having bigger JS files, not 2-3 lines where it doesn't matter) you can take advantage of compressing the JS file and front-end developers feel much comfortable working only with JavaScript in my opinion (as you could write Python, Ruby etc not only PHP - and the code could get bigger and bigger depending on what you need to do there).

share|improve this answer
2  
Totaly agree, btw I NEVER put PHP in my JS. I just see it on this site all the time. –  Greg Guida Dec 22 '11 at 17:07
    
The php code never makes it to the browser! Just the evaluated code which should now be plain javascript. So file size/compression is a non issue. Still bad practice thought! –  James Anderson Jan 6 at 9:27
add comment

The only thing I can think off that can really cause issues is when PHP errors are set to be displayed and so it shoves a load of HTML showing the PHP error into your JavaScript.

Also because its it in script it therefore doesn't show and it can sometimes take a while to realize why your script is broken.

share|improve this answer
    
great case where this causes a big error –  Greg Guida Dec 22 '11 at 17:20
add comment

Some people would argue that it's bad practice. Not because it's PHP inside JS, but because it's inline JS and so won't be cached by the browser for ease of loading the next time.

IMO it's always better to use JSON to pass variables between the 2 languages, but I guess it's up to you.

share|improve this answer
add comment

I would say that in general don't do it. However if you want to pass data from PHP -> Javascript it would not strike me as crazy to have an inline Javascript block where you have code of the form shown bellow. Here the code is just passing data from php to javascript, not creating logic on the fly or the like. The good part of doing this vs an ajax call is that the data is available as soon as the page loads and does not require an extra trip to the server.

<script>
window.config = <?php echo json_encode($config);?>;
</script>

Of course an other option is to build a javascript config file from PHP via some form of build script that will put that into a .js file.

share|improve this answer
add comment

It depends by whom, and if you ask me, yes I do consider it back practice for a few reasons. First of all, I'd prefer to have javascript code in its own JS file that the php parser would not be able to touch.

Secondly, php executes at server time only, so if you are depending on some variable in php to change your javascript, that may not work very well. If there is some page-load setting you want to control with javascript, I typically prefer to add that value to the DOM with php so that javascript can reach it when and if it wants to (in a hidden div, for example).

Finally, just for organizational purposes, this can get very annoying. It's bad enough to mix html and php (in my opinion).

share|improve this answer
add comment

I would consider this bad practice. When putting dynamic content inside script blocks you always must be aware of the fact that escaping inside a javascript context is not as simple as you would hope. If values were user-provided, it is not sufficient to html-escape them.

The OWASP XSS cheat sheet has more details, but basically you should adopt this pattern:

<script id="init_data" type="application/json">
    <?php echo htmlspecialchars(json_encode($yourdata)); ?>
</script>

Then in a separate .js file linked from your main html, load this code:

var dataElement = document.getElementById('init_data');
var jsonText = dataElement.textContent || dataElement.innerText  // unescapes the content of the span
var initData = JSON.parse(jsonText);

The reason for using a separate .js file is two-fold:

  • It is cacheable so the performance is better
  • The HTML parser is not triggered, so there is no risk of a XSS bug slipping through by someone putting a quick <?php tag in there somewhere
share|improve this answer
    
+1 for fully explaining the XSS angle! Your approach will load faster because the json is loaded before domready, but I prefer the automatic json parsing from using $.ajax or similar –  Eru Jan 6 at 11:22
add comment

Containing the PHP into a config data object goes 90% of the way but best practice is to seperate it entirely. You can use a RESTful api to only request the data that you need, it is a bit more javascript but with a few advantages.

  • Script is static and can be cached permanently
  • PHP no longer an XSS Vector
  • Complete seperation of concerns

Downsides:

  • Requires an extra HTTP request
  • more complex javascript

Script

//pure javascript
$.on('domready',function({
    //load the data
    $.get({
       url:'/charts/3D1A2E', 
       success: function(data){
           //now use the chart data here
           ChartModule.init(data);
       }
    });
})
share|improve this answer
add comment

May not be the easiest approach but you can always find a way to retrieve this variable using AJAX. If you feel the need to have lots of PHP code in your Javascript, this is probably the best way to go, though.

share|improve this answer
add comment

It's not a bad practice ONLY if it's used for javascript code initialisation, (in my WordPress themes, I initialize my javascript objects with php functions like site_url() because it's the only way to handle that (maybe we could use an ajax request to get a json, and so... but it's a pain in the ass).

Good practice:

new javascriptObject("");

Bad practice:

/* some code*/ document.get_element_by_id(); /* some code*/
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.