Laravel Junior Code Review: Security and Consistency

hello guys today we'll have another code review of more or less junior developer who created this website for mortgage and finance with things like book and appointment and with admin area and i will show you that in a minute but before we do that code review i want to show you something i've created a separate playlist because this will be my code review number 12 already so i started pretty recently but already have a few very popular code reviews so i've created a separate playlist so if you want to receive other code reviews there are 11 videos at the moment and there will be 12 and here on the right side you see all the lists it's both reviews for junior code and some reviews of more professional code and from here i will try to make it more searchable because the thing i see now those code reviews are even titled in a similar way so i don't remember which is which and maybe i can advise something to someone by pointing out to some specific code review or specific topic so i will write a more detailed description of what do we discuss during that code review and what we have learned and maybe it will be more searchable in the future and speaking about more popular code reviews i can celebrate now because my very first code review this one reached 100 000 views so this number can you imagine i haven't imagined that number or haven't expected anything like that so apparently code reviews i've touched a hot topic and you like it and you want more of that so let's continue with that and let's do another code review for this mortgage product in this review among other things you will learn how to launch artisan commands without terminal on the shared hosting probably then some security issues what happens if you forget in middleware and then how to structure things like routes views controllers and all of that typical laravel stuff so let's start with routes as usual and in the beginning of route you immediately can see why it is great to use id like phpstorm i'm not sure if it happens the same way in vs code or sublime but in this case it immediately points what is wrong so this controller isn't used in the routes this as well and storage isn't used and in this case contact controller doesn't even exist so just general advice do more cleanup the code still works but there are a lot of things that are underlined here next this part and this is pretty interesting i haven't seen much of it before but then i remembered why it is done this way so there is a route clear that performs a lot of artisan commands and basically clears a lot of stuff similarly route migrate for the migration and route storage link for the storage link command why would you do that so why would you make a web route for the artisan calls and it's usually done on shared hosting where you don't have access to the terminal directly so you cannot ssh into the machine so you create special routes that perform them for you and it's pretty convenient but the problem here is those routes are public so someone can go into the browser and type clear and launch that command without developer knowing and some of them may be a security issue so key generate should be actually run only once in the project probably because regenerating the key regenerates the sessions for the users from what i remember the logic and then migration it's probably safe but you never know what's in your migration files so my advice here would be to hide all of those under some kind of middleware at least off middleware or maybe even more permissions and off middleware is used later in the project so there are admin controllers like this one so dashboard controllers and other controllers so i would do those things as a method in dashboard controllers or other controllers because dashboard controller is protected by auth next i want to stop with this index controller and welcome if we go to that index controller it gets the data and loads front and welcome so if we open that welcome blade it looks like this so extends the layout then there's a header then there's a buddy with a lot of includes which is fine just one thing with the include here with comment this is an html comment which means that this would be actually rendered by laravel and the final code will be commented out but will be present not sure if developer meant it exactly that maybe they wanted to comment out the whole rendering of the include so in laravel it's a different syntax of commenting so there is a blade this syntax so dash dash and then if you put include in here then as you can see it's even in different color which means it's not active so it is commented on laravel level and won't be even rendered so keep in mind do you want to render or do you want to totally skip the include now in all the files for all that project so there's a front end and then there are pages like 10 or 15 pages i saw a thing repeating and i want to challenge that so privacy policy you can see section header include then whatever page feedback section header include i think there's no need to have a section here i think it should be done in a different way so we open a section we open one include which is the same for all the pages from what i've seen so far so probably if it's the same for every page it should be moved to layouts app instead unless the section header does something specific let's search for it in the layouts app header so it doesn't seem to have any variables let's take a look at here header does it have any variables or dynamic elements and so far i don't see any of them let's search for dollar sign no it's from jquery so i think that whole thing should be moved from the specific page here into here instead of yield header include like this and then in all the other pages you may skip the section altogether maybe i've missed something here but i think that section is unnecessary next in the routes if we scroll down there's a shorter way to do that so if your route has only view returned you can directly do route view and then you don't need to have callback function you just need to pass the name of the view like this with name so this will be the line why is it underlined oh i forgot the comma here so two parameters url and route view and probably name should be in new line and with such tips i feel like i'm repeating myself so every code review has something similar to other code reviews so i've mentioned that route view in the code review before just a few days ago so in the future i will think whether to repeat the same things all over for every individual or just point you to the other code review okay scroll down to the routes to the controllers now if we get to book appointment controller on the website you can download the ebook i think this is the link then you can book an appointment so this is the form and then you can subscribe to newsletter i think it's in the footer somewhere and what else you can do you can give the feedback and my challenge here is the naming of the controller so all those four things are attached to book appointment controller whereas in fact the appointment happens only in one case appointment book so that book appointment controller actually takes care of just store which is store appointment and it is good but then store subscribers store feedback store ebook download so i would challenge that so those ebook downloads newsletter and feedback should be in a separate controller like home controller maybe or something like that so book appointment is specifically for appointments and everything else is for everything else separately i know it's just a naming issue but i'm challenging that because other developers in the future may read that code and think that newsletter has something to do with appointment booking and it's not the case they are totally different things next if we scroll down to the admin group of controllers i see inconsistency and in general it's a typical thing for junior code so this is my like 12th review or something junior developers are really inconsistent in the way they write code so one part is written in one way another part is in another way and both work and we move on and don't touch it i think cleanup of the code for readability is a quite important thing and it actually may raise your level from junior to mid or to senior so you clean up after yourself and here we have controller used in laravel 8 syntax and down below we go back to old syntax from clickable controllers where i can click with my php store to not clickable controllers and i cannot click here so why don't we do that in one way and reference dashboard controller like we did here and then that dashboard controller is generated in the use section or in fact it was already here remember in the very beginning of the video so now we actually use that so i would challenge that we need to stick to one syntax so for example contact controller and there are two controllers contact and admin contact controller maybe that was the reason why it wasn't used this way let's try another one address controller address controller it's one address controller and then i would suggest to go through other controllers and probably move that to the use section on top and it would be the same syntax then now i see another thing that i see a lot in junior code reviews that route resources are used for things that are not actually resource let's take a look at dashboard controller what it has inside index and then other resource methods but do you see any code inside of that show edit update destroy they're all empty and dashboard is what is just one dashboard so there's no create dashboard or edit dashboard so instead of route resource in this case it should be route get with controller class method index and delete all the methods except for what you actually use and this controller is much shorter than so i haven't checked all the controllers but i'm pretty sure a few of them are also not resource controllers but just one or two methods and now i will show you why it is beneficial to add the middleware in the routes instead of separate controllers so in here you have slider controller and other controllers and the middleware auth is assigned in the controller the tricky part here is i will show you first this is an admin area it's really simple so i've created a user for myself manually in the database and we have an error with dashboard index i forgot while refactoring to assign a name here so route get should be name dashboard index because it came from the route resource before now we have to manually assign it refresh and here's our dashboard and it's all fine and working and you can manage ebooks appointments and all of that but look what happens if i log out or in fact how can i log out that's an interesting thing so there's no log out button or link probably they think they don't need it but here's a trick for you by the way what if you don't have logout button and in laravel you cannot just go slash logout because it's a form it's a post request so the trick for you is to clear the cookies clear the cookies of that particular website remove the cookie done and if we refresh laravel test login and try to go to admin dashboard we will be redirected out to login so we are logged out but now interesting thing let's take a look at other controllers and for example ebook controller do you see any middleware here i don't so let's try to launch that ebook remember we're logged out slash admin slash ebook and we're into the dashboard so this is a security issue and i think that should convince you that it's better to assign a middleware here like this middleware off instead of doing that separately in the controllers because when creating a new controller or maybe other developer will create that controller they may forget to add that middleware and final thing i wanted to show you is how to not assume that the data is in a database so we have contact controller for the admin area and it is used probably to manage home stuff like contact and then there's address there's social media similar controllers so there is address controller which takes the address with a d1 and then there's a store or update method edit an update actually which just updates the address record and the contact record i'm assuming it's for the home page but what actually happens here you take the contact with id1 do we have any contact for a new project which i've just downloaded and installed database seeds we don't have any contacts here and in the database if we go to contacts table it's empty same for addresses so if i go to that admin area page click on contact and i see an error undefined offset so there are two things i can advise here first do not assume that the data is in the database if you do something like this then you need to check if there is a contact probably it should be get it's not even yet it should be first actually because we have one contact probably and let's actually take a look at contact index how it is referenced here okay contact information form email address and the value is contact zero of course it's not an array there's one contact from what i understand in this logic so it should be first here and then we don't need zero here and then it should be better to do first or fail which means that if the contact is not found it will not show the error like this in laravel it should show 404 not found or even better in this case that page doesn't really say anything to the user so first and then if no contact we can redirect somewhere with specific error message or you just can return contact not found check the database without any design so if we refresh now we will see that error or probably a better way would be to have it in the blade admin contact index so you check in here at the very top so if contact then you have all that form otherwise else and if no contact found like this let's refresh and we have no contact found so that's better because they can navigate to wherever they want from here and then i would challenge the whole idea of having an index for a separate contact than having an index for specific address controller and for others i think i have a theory that those all should be in the settings controller somewhere so should be settings or config table in the database and then there would be one form to manage contact details address and other so it would be total lag of 10 fields or something that would be in the same database table in one controller and on one page to manage instead of those three or four separate pages so that's all i have for this review these are the things i found it doesn't mean that the code is bad i found a few issues here and there but those who send me the code for review and this time it was from twitter be prepared to have some criticism from me but it's great from your side that you are brave enough to have your code challenged and only in this way we learn together so i learned actually from these videos about the topics that i need to shoot other videos in the future because i see weak spots in some junior code and what do i need to repeat and you all learn not only that person from that project but you all i hope you learned something here and there how to make your code better if you want more code reviews like this one subscribe to the channel and if you want to support me on my mission financially check out one of the three products you can see on the screen my live market set of components if we start from the bottom then we have my courses now it's 16 courses on teachable and then you can use laravel quick admin panel generator see you guys in other videos

Channel: Laravel Daily

Views: 25,598

Rating: 4.9832635 out of 5

Keywords:

Id: oP7XRJZG-90

Length: 17min 28sec (1048 seconds)

Published: Sat Mar 13 2021