Dynamically generated menu based on privileges

Question

 <?php 
            $mnquery=$ob->showDataall("select * from menus where parent_id='0' and delet='0'");
            foreach($mnquery as $value)
                {
                    $selquery=$ob->showDataall("select * from menu_permission where staff_id='".$_SESSION['user_id']."'");
                    foreach($selquery as $nquery)
                        {
                            foreach(explode(",",$nquery['permission']) as $mnu)
                                {
                                    if($value['menu_id']==$mnu)
                                        {
                                    ?>
        <li><a <?php if($value['menu_hyper']!='#'){ echo 'href="'.$value['menu_hyper'].'"';}?>><i class="fa fa-bars"></i><?php echo $value['menu_name']; if($value['menu_hyper']=='#'){?><span class="fa fa-chevron-down"></span><?php }?></a>
                                    <?php
                                        $submnquery=$ob->showDataall("select * from menus where parent_id='".$mnu."' and parent_id!='0' and delet='0'");
                                        if(!empty($submnquery))
                                        foreach($submnquery as $subvalue)
                                            {
                                                $selquery=$ob->showDataall("select * from menu_permission where staff_id='".$_SESSION['user_id']."'");
                                                foreach($selquery as $nquery)
                                                    {
                                                        foreach(explode(",",$nquery['permission']) as $mnua)
                                                            {
                                                if($subvalue['menu_id']==$mnua)
                                                    {
                                    ?>
                                                    <ul class="nav child_menu" style="display: none">
                                                        <li><a <?php if($subvalue['menu_hyper']!='#'){ echo 'href="'.$subvalue['menu_hyper'].'"';}?>><?php echo $subvalue['menu_name']; if($subvalue['menu_hyper']=='#'){?><span class="fa fa-chevron-down"></span><?php }?></a>





                                                  <?php
                                                            $chldmnquery=$ob->showDataall("select * from menus where parent_id='".$subvalue['menu_id']."' and delet='0'");
                                                            if(!empty($chldmnquery))
                                                            foreach($chldmnquery as $chldvalue)
                                                                {
                                                                    $selquery=$ob->showDataall("select * from menu_permission where staff_id='".$_SESSION['user_id']."'");
                                                foreach($selquery as $nquery)
                                                    {
                                                        foreach(explode(",",$nquery['permission']) as $mnub)
                                                            {
                                                                    if($chldvalue['menu_id']==$mnub)
                                                                    {
                                                        ?>
                                                            <ul class="nav child_menu" style="display: none">
                                                                <li><a href="<?php echo $chldvalue['menu_hyper'];?>"><?php echo $chldvalue['menu_name'];?></a></li>
                                                            </ul>
                                                        <?php
                                                                    }
                                                                }
                                                    }
                                                                }
                                                        ?>      

                                                        </li>
                                                    </ul>
                                <?php
                                                    }
                                            }
                        }
                                            }
                                ?>
                                    </li>
                                <?php
                                        }
                                }
                        }
                }
              ?>
</ul>

Here is my Table named 'menus'

 menu_id       menu_name            menu_hyper                 parent_id    delet
    1         Master Settings            #                         0         0
    2         Add New Menu       ../pages/menu_add.php             1         0
    3         Menu Permission    ../pages/menu_permissions.php     1         0
    4         Role               ../pages/add_role.php             1         0
    5         Staff              ../pages/add_staff.php            1         0
    6         Add Product        ../pages/product_add.php          0         0

Here is my other table named 'menu_permission'

 id    staff_id    permission
   1       1        1,2,3,4,5,6
   2       2        1,4

Here is my function showDataall

public function showDataall($result) 
    {
        $q = $this->conn->prepare($result) or die("failed!");
        $q->execute();
        while ($r = $q->fetch(PDO::FETCH_ASSOC)) 
        {
            $data[] = $r;
        }
        return $data;
    }

This code is working.

Can anyone help me to simplify this code? I think it's more complex than it needs to be, and it's totally confusing me.

It's also loading very slowly, probably because I use so many loops in it.

Answer 1

Security

You are very likely open to SQL injection. You should never directly put variables into queries, and you really need to use prepared statements.

You are also likely open to persistent XSS, which may or may not matter, depending on your application.

Formatting

• Your lines are up to 273 characters long, which is way too much. You should aim for 80 characters, 100 max, otherwise your code is really hard to read.
• You should upper-case SQL keywords to increase readability.
• Your indent style is very uncustomary (it's also not always consistent). Try to use one of the more often used styles.

Structure

If code ends like this:

                                                    <?php
                                                                }
                                                            }
                                                }
                                                            }
                                                    ?>      

                                                    </li>
                                                </ul>
                            <?php
                                                }
                                        }
                    }
                                        }
                            ?>
                                </li>
                            <?php
                                    }
                            }
                    }
            }
          ?>

Then you know that it is just awfully structured. It is way too nested, and I have no idea which bracket closes what.

You also have quite a bit of duplication, which you could remove by introducing functions.

Personally, I would rewrite your whole code from scratch, as there are so many things that aren't quite right. When you do that, try to first create smaller units of code, which you then combine. And use functions to simplify your code, so that you don't have to keep everything in mind at the same time. Functions may be: getMenuPermissions($staffID), getMenuesByParent($parentID), showMenu($menu), showMenuLink, ... This is just a quick first idea, your code is a bit too confusing for me to suggest all the proper functions you may need.

Naming

Good variable names are very important for readability. All your variable names are too generic or slightly wrong, for example:

• mnquery: this isn't a query, it's an array of results. It should be named menues.
• value: value of what? This name doesn't tell me anything. As it is one item out of the menues array, it should be called menue.
• selquery: again, not a query. and sel isn't helpful. what does it select? menuePermissions.
• nquery: not a query, and what does n stand for? never abbreviate variable names. menuePermission would be a good name.
• mnu: what's this? menue? But is it? or is it a permission? or a menuePermissionId? It's hard for me to tell, but whatever it is, it should be named that.
• other names that are too generic: submnquery, nquery (again), mnua (what does a mean?), subvalue (value of what?), chldvalue, chldmnquery, and so on.

Answer 2

First I have simplyfied your code to see what it does. I have left only the syntax.

It is important that you are able to read you own code in case you have to fix bugs some day!

And I can tell you. The day will come and then you have no idea what you have done there.

<?php

foreach( $mnquery as $value ){  # $mnquery = menu items
foreach($selquery as $nquery){
foreach(/* permissions */ as $mnu){
  if($value['menu_id']==$mnu){
    # here you should use if and else
    if($value['menu_hyper']!='#'){#add hyperlink}
    if($value['menu_hyper']=='#'){#add span}
    #database select 2
    if(!empty($submnquery)) foreach($submnquery as $subvalue){
      #database select 3
      foreach($selquery as $nquery){
      foreach(explode(",",$nquery['permission']) as $mnua){
        if($subvalue['menu_id']==$mnua){
        if($subvalue['menu_hyper']!='#'){}
        if($subvalue['menu_hyper']=='#'){}
        #database select 4
        if(!empty($chldmnquery)) foreach($chldmnquery as $chldvalue){
                        #database select 5
          foreach($selquery as $nquery){
          foreach(explode(",",$nquery['permission']) as $mnub){
          if($chldvalue['menu_id']==$mnub){ /* buid element */ }
# then 11 closing brackets...
# 7x foreach
# 9x if

what you have done is this:

foreach(){
  foreach(){
    foreach(){
      foreach(){
                # ... foreach, foreach, foreach ...
      }
    }
  }
}

I am sure you will find a better solution. Maybe by using functions like:

function get_menu_items(){}

Try to store your results in Arrays and use them in a better order.

---

This will do exactly what your code does the same with one change in the database (is described in the code commands).

<?php


# get all content from the table ONCE and store them in an array.
# Think about the runtime. The database will need 0.001 sec per select
# Lets say 10 users loading the page at the same time:

# 10 users * 6 requests to run this script * 0.001 seconds per request
# 0,06 seconds will ONLY THE SQL SERVER NEED ( in case you have a fast server ).


# in this case I stored the menu table in an array with this structure:
# changed menu_name to name because i already know that this is the menu.
$menu = array(
    [
        "id" => 1,
        "name" => "Master Settings",
        "hyper" => "#",
        "parent_id" => 0
    ]
);

# here you have to write some code to define the user permissions
# admin = true
# everyone else = false
# TODO define user permissions
$is_admin = true; # or false


# this function builds the item
function build_menu_item($link, $title, $id){
    #   \n  -   will print a new line in the html source code
    #   \t  -   will print a tab in the source code

    # store everything in $item
    $item  = "\t<li class=\"menu_item\" id=\"menu_item_$id\">";
    $item .= "<a href=\"$link\">\n";
    $item .= "\t\t<i class=\"fa fa-bars\" id=\"fa_bars_$id\"></i>$title\n";

    # check the link to build the additional <span> tag
    if( $link == "#" )
    {
        $item .=  "\t\t<span";
        $item .=  "class=\"fa fa-chevron-down\" ";
        $item .=  "id=\"fa_chevron_down_$id\">";
        $item .=  "</span>\n";
    }

    $item .= "\t</a></li>\n";

    # now echo $item or return $item
    echo $item;
    #return $item;
}

# This is the script:


/*
this is the same as:

if( $user_permissions == "admin" ){
    $is_admin = true;
}else{
    $is_admin = false;
}

*/
echo "\n<ul class=\"menu\" id=\"menu\">\n";

#loop for every item
for ( $i=0; $i < count($menu); $i++ )
{
    #parent_id = menu[ i as current_item ][ parent_id ]
    $parent_id = $menu[$i]['parent_id'];
    $menu_hyper = $menu[$i]['hyper'];
    $menu_name = $menu[$i]['name'];

    # if there is only one line behind you dont have to write {}
    if( $parent_id == 1 && $is_admin OR $parent_id == 0)
        build_menu_item($menu_hyper, $menu_name, $i);
}

echo "</ul>\n";

----------

How it works

The Following code is exactly the right order and you can just run this script without database to see what it does.

---

What I want to do: I want to build a menu with Items. Items are only visible if the user has permissions.

what do I need to build an item? the value that will be displayed on the item

function item( $value ){
    # how will the item look like?
    # just a simple item with text

    $item = "<li>" . $value . "</li>";
    echo $item;
}

now I have to get the items with the permissions of every item

these are my items:

| value     | permissions |
|-----------|-------------|
| Home      |      0      |
| About     |      0      |
| Private   |      1      |
| Settings  |      2      |

And I have 3 types of users:

| guest     | 0 |
| user      | 1 |
| admin     | 2 |

guests will see Home and About user will see Home, About and Private admin will see Home, About, Private and Settings

Here are the arrays:

$_MENU = array(
    [   "Home",     0   ],
    [   "About",    0   ],
    [   "Private",  1   ],
    [   "Settings", 2   ]
);

$_USERS = array(
    0 => [ "user_id" => 1, "name" => "Peter Bishop", "permissions" => 1 ],
    1 => [ "user_id" => 2, "name" => "Walter Bishop", "permissions" => 2 ]
);

Let's say I am logged in with the user id 2 and I am Walter Bishop

$user_id = 1;

We have everything done to start building the items:

foreach ($_MENU as $item) {
    # I have 2 values in each menu item:
    $menu_value = $item[0];     # the name
    $menu_permissions = $item[1];   # the permissions

    # I am Walter Bishop with user id 1 do I have permissions?
    $user_permissions = $_USERS[ $user_id ][ "permissions" ];

    # if the user_permissions greater or equal to
    # menu_permissions then...

    if( $user_permissions >= $menu_permissions )
    {
        # ... prin the item
        item( $menu_value );
    }
}

This is the output:

<li>Home</li>
<li>About</li>
<li>Private</li>
<li>Settings</li>

In case you don't understand what the code does your problem is outside of your project.

Then you may used this order:

1. Start a Project
2. Learn some basics of PHP
3. Copy and paste from the internet
4. Wondering why it does not looks so good
5. Ask the internet why it does not looks so good

This order will return

FATAL ERROR on task 3

Here is the Correct order

1. Learn PHP basics ( dont watch youtube videos... they are often wrong )
2. Do some practise
3. Learn more PHP ( use the Documentation on php.net )
4. Do more prectice
5. Start the Project ( if something went wrong go back to task 3 )

If you have chosen the 2. task order then the following will be no problem for you. ( I really hope so... Otherwise you wasted my time because of your own laziness. )