Don’t pay too much attention to @igor_g. It’s clear they don’t understand heredoc. Any reasonable editor will do proper syntax highlighting and show errors for heredoc strings.
Using a template system such as Twig is a very common alternative. More common in fact then using heredoc. Just takes a bit more effort to install and get started.
I have checked my PHPStorm - that’s true, it makes highlights on Heredoc. A couple years before I worked with Eclipse, and there was no highlights. So, I don’t know how it generally looks like, with different editors. But it’s not important…
HTML-markup directly in PHP-class - is very bad coding style. And I thought it’s cleary and need no more discussion.
Actually, HTML-markup needs not only param-placeholder, but also control-operators: if…else, foreach. In string that is impossible.
And why? If we have e.g. ASP in phtml-format…
public function render()
{
ob_start();
include 'template.phtml';
return ob_get_clean();
}
Summary… Just forget Heredoc as Markup-rendering concept. Heredoc exists not for this at all.
@igor_g Your answers are starting to confuse me…
@ahundiak I took a break for the remainder of the day yesterday. So let me see if I can figure out that code you posted and how to make it work with my database:
<?php
$app = new NoEdit();
$app->run();
class NoEdit
{
public function run()
{
if (isset($_POST['btn_save'])) {
var_dump($_POST);
die();
}
$currentUserId = 42; // $_SESSION['id'];
$rowUser = [
'id' => 42,
'status' => 33,
'first_name' => 'Tom <"> Jerry',
];
$html = $this->render($currentUserId,$rowUser);
echo $html;
}
protected function escape(string $value) : string
{
return htmlspecialchars($value, ENT_COMPAT);
}
private function render($currentUserId,$rowUser)
{
$html = <<<EOT
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Test</title>
</head>
<body>
{$this->renderContent($currentUserId,$rowUser)}
</body>
</html>
EOT;
return $html;
}
private function renderContent($currentUserId,$rowUser)
{
return <<<EOT
<form method="POST">
<input type="text" name="first_name" value="{$this->escape($rowUser['first_name'])}" />
{$this->renderStatus($currentUserId,$rowUser['id'],$rowUser['status'])})
<input type="submit" name="btn_save" value="Save">
</form>
EOT;
}
private function renderStatus($currentUserId,$rowUserId,$status)
{
if ($currentUserId === $rowUserId) {
return <<<EOT
<select disabled class="form-control" id="status" name="status">
<option selected value="1">Active</option>
<option value="0">Inactive</option>
</select>
<input type="hidden" name="status" value="{$status}" />
EOT;
}
// This is very hokey, you really should have a loop here
// Leave that as an excersize
if ($status === 1) {
return <<<EOT
<select class="form-control" id="status" name="status">
<option selected value="1">Active</option>
<option value="0">Inactive</option>
</select>';
EOT;
}
return <<<EOT
<select class="form-control" id="status" name="status">
<option value="1">Active</option>
<option selected value="0">Inactive</option>
</select>';
EOT;
}
}
(I changed the class name from “App” to “NoEdit” just so I can recognize what it is used for.) You first have a var_dump() in place. What is this for? Doesn’t that just display information?
if (isset($_POST['btn_save'])) {
var_dump($_POST);
die();
}
Then you have an array which would contain the id, status and first name of that particular user:
$currentUserId = 42; // $_SESSION['id'];
$rowUser = [
'id' => 42,
'status' => 33,
'first_name' => 'Tom <"> Jerry',
];
Since the data you posted is fake, I believe I would have to change this to:
$currentUserId = $_SESSION['id'];
$rowUser = [
'id' => $rowUser['id'],
'status' => $rowUser['status'],
'first_name' => $rowUser['first_name'],
];
(Correct me if I’m wrong on that…)
The rest I don’t quite understand. You have a lot of private functions that generate html with a lot of renders in them. Not sure what any of that means:
$html = $this->render($currentUserId,$rowUser);
echo $html;
}
protected function escape(string $value) : string
{
return htmlspecialchars($value, ENT_COMPAT);
}
private function render($currentUserId,$rowUser)
{
$html = <<<EOT
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Test</title>
</head>
<body>
{$this->renderContent($currentUserId,$rowUser)}
</body>
</html>
EOT;
return $html;
}
private function renderContent($currentUserId,$rowUser)
{
return <<<EOT
<form method="POST">
<input type="text" name="first_name" value="{$this->escape($rowUser['first_name'])}" />
{$this->renderStatus($currentUserId,$rowUser['id'],$rowUser['status'])})
<input type="submit" name="btn_save" value="Save">
</form>
EOT;
}
private function renderStatus($currentUserId,$rowUserId,$status)
{
if ($currentUserId === $rowUserId) {
return <<<EOT
<select disabled class="form-control" id="status" name="status">
<option selected value="1">Active</option>
<option value="0">Inactive</option>
</select>
<input type="hidden" name="status" value="{$status}" />
EOT;
}
// This is very hokey, you really should have a loop here
// Leave that as an excersize
if ($status === 1) {
return <<<EOT
<select class="form-control" id="status" name="status">
<option selected value="1">Active</option>
<option value="0">Inactive</option>
</select>';
EOT;
}
return <<<EOT
<select class="form-control" id="status" name="status">
<option value="1">Active</option>
<option selected value="0">Inactive</option>
</select>';
EOT;
}
}
@jmyrtle Apologies for overwhelming you. I tossed a bunch of new stuff at you. Slow day for me.
Recall that the original problem with your code was that it was not taking into account the fact that select elements are not actually posted for disabled elements resulting in null values in your database. You seemed to be having difficulty understanding and correcting this. Which is fine. You have a pretty big file and it can be hard to isolate things.
So what I first did was to try and demonstrate the problem with a simple test file. That was my first 24 line bit of code. As you surmised, all that var_dump does is to show the value of a variable. It’s a quick and easy way to see that $_POST did not contain a value for status. I then showed one way to to ensure it always will have a value using a hidden element.
You can go back to your original code, fix this issue and that particular problem should be resolved. And then move on. Probably your best choice.
But we did have some further discussion which led me to post 82 line second example. The idea was to show a more ‘organized’ approach to building pages. I suspect it failed miserably.
Going back to the big picture, one fundamental concept that applies across most programming is to try and isolate the view (i.e. html rendering) from the rest of your code. The view should not care about where the data it needs is coming from and should be separated as much as possible from the rest of your code. In my case, the render method represents the view.
The view needs to know the current user’s id. It does not need to know that it comes from a session or maybe some place else. Hence, in my test code, I just set the value and passed it to render. In your code you would pull it from the session. The point is that the view does not care. It just needs the value.
Likewise the view does not care where $rowUser comes from. I certainly did not want to setup a complete database and query from it. So I just faked one. In your code, you would actually pass the $rowData from your query. The view won’t care.
Again, this is all just about separating your view from the rest of your code. render($currentUserId,$rowUser) tells us exactly what your view needs.
As far as the multiple render methods, another key approach to programming is to try and break things down into smaller chunks of code so you can see what is going on.
html - render
body
form - renderContent
select status - renderStatus
The render method main job is generate the html page skeleton. Basically the html header section. It’s the sort of thing that will get repeated for pages so you kind of want to isolate it. Eventually you would want to share this code across multiple pages.
The renderContent generates html for a given page. In this case it is just a form. Many form elements such as inputs and buttons are fairly simple so we can just render them directly.
However, the status element needs a bit more work. So in keeping with the divide and conquer approach, we give it it’s own renderStatus ($currentUserId,$userId,$status) method. Notice that $rowUser is not passed. Instead, we pass it the exact variables that it needs to do it’s job. It’s a subtle point but it can makes it easier to understand exactly what the method is doing.
I was not comfortable with the actual renderStatus code I posted because it basically renders the same thing three time based on some conditions. It’s not going to work well if you have more than two options or make other changes. I was trying to follow your coding style and not add yet another new concept. I could post an updated more generic method but again that would be even more new code and concepts.
In conclusion, fix your original code and move on if you like.
If you like to continue this discussion then perhaps open up a new topic and I’ll be glad to engage. I think we are just confusing the original poster.
I will say that probably 95% of developers who understand the problem will agree with you and they use your flat file procedural approach to templates. I use them myself most of the time. But I am one of those small minorities who feel that a more nuanced, object oriented approach is sometimes called for. Like Facebook does.
As you previously stated:
From what everybody is saying (which I really appreciate your comments and patience with me btw…) that’s gonna be what the fix is.
if($_SESSION['id'] == $rowUser['id']) {
echo '<select disabled class="form-control" id="status" name="status">
<option selected value="1">Active</option>
<option value="0">Inactive</option>
</select>';
echo '<p style="color: #a40000;">The user status cannot be changed</p>';
} else {
if($rowUser['status'] === '1') {
echo '<select class="form-control" id="status" name="status">
<option selected value="1">Active</option>
<option value="0">Inactive</option>
</select>';
} elseif ($rowUser['status'] === '0') {
echo '<select class="form-control" id="status" name="status">
<option value="1">Active</option>
<option selected value="0">Inactive</option>
</select>';
}
}
Here is my original code where I started. Basically, if the id of the session is equivalent to the id of the user, then I need to show the current value and prevent the user from editing the option. Otherwise, we can change these values (this would be for different users). I still want to keep this in the code if at all possible.
As everyone has said, there’s a problem: A disabled field does not submit any values. So what I need to do is set it as a hidden value like this:
<input type="hidden" class="form-control" id="role_id" name="role_id"
placeholder="" value="<?php print($rowUser['role_id']); ?>" maxlength="255"
autocomplete="off" />
However, because it is an if statement in PHP, I will have quotation issues because of this section:
... value="<?php print($rowUser['role_id']); ?>" ...
There is a mix of single and double quotes. Usually I would use echo in if statements, but this isn’t going to work. If I use an echo with double quotes, the string will end when it sees the next set of quotes in the string. If I use single quotes, the string will end in the PHP string where the HTML value is defined.
So then, I tried using heredoc,
echo <<<EOT <input type="hidden" class="form-control" id="role_id" name="role_id"
placeholder="" value="<?php print($rowUser['role_id']); ?>" maxlength="255"
autocomplete="off" />
EOT;
but that gives me a syntax each time I try to use it.
The way I look at it now… I’m kinda out of options…
['id'] == $rowUser['id']) {
echo '<select disabled class="form-control" id="status" name="status">
<option selected value="1">Active</option>
<option value="0">Inactive</option>
</select>';
$hidden = <<<EOT
<input type="hidden" name="status" value="{$rowUser['status']}" />
EOT;
echo $hidden;
echo '<p style="color: #a40000;">The user status cannot be changed</p>';
} else {
You are mixing up your role_id select with your status select.
You copied a bunch of unnecessary attributes into your hidden element. They won’t hurt but they will confuse things.
And as I mention a few posts ago, I suspect your error message is probably coming from a different block of code that you forgot you changed. Make sure your code is running without syntax errors before trying to add the hidden element.
And if you want to stay away from heredoc then try:
echo '<input type="hidden" name="status" value="'
. $rowUser['status'] . '" />';
The . string concatenation operator can resolve the quoting issues. sprintf could also be used.
Sorry, yes I did. I copied the wrong code in my last post… But if it helps, $role_id and $status are coded the same way, so in theory, it should work on both.
So if I use this, would it go before or after the disabled select (or does it matter)?
Best to put it after. In this particular case it does not really matter because the select element is always disabled.
That did it! Thanks for your help everyone! I’m definitely not the best when it comes to PHP, but I’m slowly learning as I go. I appreciate all the input you guys give me.
I think you went a wrong way. Object oriented approach in thema Markup-rendering optimally to realise with…
…DOM-objects using,
…Component-builded web-sites.
Actually, exactly like I do in my framework.
No idea what you are saying but open a new thread and I’ll be glad to engage.
This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.