Is this a good practice to load controllers
-1
$begingroup$
I'm new to PHP and I'm learning mostly,
Is this a good practice to load controllers in PHP? (for a small and lightweight app)
Does it have any vulnerabilities? or it's fine and I can move on to write my controllers?
<?php
// application path (outside public_html)
define('APP', '../application/');
// controllers
define('CONTROLLER', APP.'controller/');
// Default controller
$controller = 'default';
// Check if page is set (e.g. index.php?page=login)
if(isset($_GET['page'])){
// Get controller name and remove possible / from the end
$controller = rtrim($_GET['page'],'/');
}
// Set controller path
$file = CONTROLLER."{$controller}.php";
// Check page
if(preg_match('/^[A-Za-z0-9_]+$/', $controller) !== 1 OR ! file_exists($file)){
// Error
require_once('404.php');
die();
}
// Necessary stuff
require_once(APP.'system.php');
// Load the controller
require_once($file);
// Nothing more comes in index.php
P.S. This is the bare bone with some extra stuff removed
P.S.S. I'm not trying to go with MVC, just some way to load pages securely under index.php
php
asked 4 hours ago
J. DoeJ. Doe
11
New contributor
J. Doe is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
$endgroup$
|
show 2 more comments
-1
$begingroup$
I'm new to PHP and I'm learning mostly,
Is this a good practice to load controllers in PHP? (for a small and lightweight app)
Does it have any vulnerabilities? or it's fine and I can move on to write my controllers?
<?php
// application path (outside public_html)
define('APP', '../application/');
// controllers
define('CONTROLLER', APP.'controller/');
// Default controller
$controller = 'default';
// Check if page is set (e.g. index.php?page=login)
if(isset($_GET['page'])){
// Get controller name and remove possible / from the end
$controller = rtrim($_GET['page'],'/');
}
// Set controller path
$file = CONTROLLER."{$controller}.php";
// Check page
if(preg_match('/^[A-Za-z0-9_]+$/', $controller) !== 1 OR ! file_exists($file)){
// Error
require_once('404.php');
die();
}
// Necessary stuff
require_once(APP.'system.php');
// Load the controller
require_once($file);
// Nothing more comes in index.php
P.S. This is the bare bone with some extra stuff removed
P.S.S. I'm not trying to go with MVC, just some way to load pages securely under index.php
php
asked 4 hours ago
J. DoeJ. Doe
11
New contributor
J. Doe is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
$endgroup$
$begingroup$
Downvote reason please? This is "Code Review" and as a beginner I simply hoped for a professional to kindly take a look at this if possible :(
$endgroup$
– J. Doe
3 hours ago
$begingroup$
$pathis undefined in your question's code. Which meansfile_exists($path)is always false in this example. And if it made it far enough this would fail toorequire_once($path);
$endgroup$
– ArtisticPhoenix
3 hours ago
$begingroup$
@ArtisticPhoenix I simplified variable/constant names for here, it was a typo, thanks for pointing that out
$endgroup$
– J. Doe
3 hours ago
$begingroup$
That was what I figured. - no problem. Otherwise it looks ok, you may want to name our constants something more "verbose" so they don't cause any conflicts. I would probably do this as a constant$controller = 'default';thenif(!defined('DEFAULT_CONTROLER')) define('DEFAULT_CONTROLER', 'default')That way if you assign it before including this you can override it. But thats not a big deal.
$endgroup$
– ArtisticPhoenix
3 hours ago
$begingroup$
P.S.S. I'm not trying to go with MVC- You don't have to go full on MVC, the benefit is that with an auto loader you can avoid touching the path at all. So there is no possiblillity for Directory Transversal../../../config.phpetc. I have this Router and AutoLoader - the router I did for a SO question, so it's less robust then the autoloader. But it loads based on the Class by instantiating instead of building a path. The autoloader has fair documentation, and its simple
$endgroup$
– ArtisticPhoenix
3 hours ago
|
show 2 more comments
-1
-1
-1
$begingroup$
I'm new to PHP and I'm learning mostly,
Is this a good practice to load controllers in PHP? (for a small and lightweight app)
Does it have any vulnerabilities? or it's fine and I can move on to write my controllers?
<?php
// application path (outside public_html)
define('APP', '../application/');
// controllers
define('CONTROLLER', APP.'controller/');
// Default controller
$controller = 'default';
// Check if page is set (e.g. index.php?page=login)
if(isset($_GET['page'])){
// Get controller name and remove possible / from the end
$controller = rtrim($_GET['page'],'/');
}
// Set controller path
$file = CONTROLLER."{$controller}.php";
// Check page
if(preg_match('/^[A-Za-z0-9_]+$/', $controller) !== 1 OR ! file_exists($file)){
// Error
require_once('404.php');
die();
}
// Necessary stuff
require_once(APP.'system.php');
// Load the controller
require_once($file);
// Nothing more comes in index.php
P.S. This is the bare bone with some extra stuff removed
P.S.S. I'm not trying to go with MVC, just some way to load pages securely under index.php
php
asked 4 hours ago
J. DoeJ. Doe
11
New contributor
J. Doe is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
$endgroup$
I'm new to PHP and I'm learning mostly,
Is this a good practice to load controllers in PHP? (for a small and lightweight app)
Does it have any vulnerabilities? or it's fine and I can move on to write my controllers?
<?php
// application path (outside public_html)
define('APP', '../application/');
// controllers
define('CONTROLLER', APP.'controller/');
// Default controller
$controller = 'default';
// Check if page is set (e.g. index.php?page=login)
if(isset($_GET['page'])){
// Get controller name and remove possible / from the end
$controller = rtrim($_GET['page'],'/');
}
// Set controller path
$file = CONTROLLER."{$controller}.php";
// Check page
if(preg_match('/^[A-Za-z0-9_]+$/', $controller) !== 1 OR ! file_exists($file)){
// Error
require_once('404.php');
die();
}
// Necessary stuff
require_once(APP.'system.php');
// Load the controller
require_once($file);
// Nothing more comes in index.php
P.S. This is the bare bone with some extra stuff removed
P.S.S. I'm not trying to go with MVC, just some way to load pages securely under index.php
php
php
asked 4 hours ago
J. DoeJ. Doe
11
New contributor
J. Doe is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
asked 4 hours ago
J. DoeJ. Doe
11
New contributor
J. Doe is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
edited 2 hours ago
J. Doe
asked 4 hours ago
J. DoeJ. Doe
11
New contributor
J. Doe is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
asked 4 hours ago
J. DoeJ. Doe
11
asked 4 hours ago
J. DoeJ. Doe
11
11
New contributor
J. Doe is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
New contributor
J. Doe is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
J. Doe is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
$begingroup$
Downvote reason please? This is "Code Review" and as a beginner I simply hoped for a professional to kindly take a look at this if possible :(
$endgroup$
– J. Doe
3 hours ago
$begingroup$
$pathis undefined in your question's code. Which meansfile_exists($path)is always false in this example. And if it made it far enough this would fail toorequire_once($path);
$endgroup$
– ArtisticPhoenix
3 hours ago
$begingroup$
@ArtisticPhoenix I simplified variable/constant names for here, it was a typo, thanks for pointing that out
$endgroup$
– J. Doe
3 hours ago
$begingroup$
That was what I figured. - no problem. Otherwise it looks ok, you may want to name our constants something more "verbose" so they don't cause any conflicts. I would probably do this as a constant$controller = 'default';thenif(!defined('DEFAULT_CONTROLER')) define('DEFAULT_CONTROLER', 'default')That way if you assign it before including this you can override it. But thats not a big deal.
$endgroup$
– ArtisticPhoenix
3 hours ago
$begingroup$
P.S.S. I'm not trying to go with MVC- You don't have to go full on MVC, the benefit is that with an auto loader you can avoid touching the path at all. So there is no possiblillity for Directory Transversal../../../config.phpetc. I have this Router and AutoLoader - the router I did for a SO question, so it's less robust then the autoloader. But it loads based on the Class by instantiating instead of building a path. The autoloader has fair documentation, and its simple
$endgroup$
– ArtisticPhoenix
3 hours ago
|
show 2 more comments
$begingroup$
Downvote reason please? This is "Code Review" and as a beginner I simply hoped for a professional to kindly take a look at this if possible :(
$endgroup$
– J. Doe
3 hours ago
$begingroup$
$pathis undefined in your question's code. Which meansfile_exists($path)is always false in this example. And if it made it far enough this would fail toorequire_once($path);
$endgroup$
– ArtisticPhoenix
3 hours ago
$begingroup$
@ArtisticPhoenix I simplified variable/constant names for here, it was a typo, thanks for pointing that out
$endgroup$
– J. Doe
3 hours ago
$begingroup$
That was what I figured. - no problem. Otherwise it looks ok, you may want to name our constants something more "verbose" so they don't cause any conflicts. I would probably do this as a constant$controller = 'default';thenif(!defined('DEFAULT_CONTROLER')) define('DEFAULT_CONTROLER', 'default')That way if you assign it before including this you can override it. But thats not a big deal.
$endgroup$
– ArtisticPhoenix
3 hours ago
$begingroup$
P.S.S. I'm not trying to go with MVC- You don't have to go full on MVC, the benefit is that with an auto loader you can avoid touching the path at all. So there is no possiblillity for Directory Transversal../../../config.phpetc. I have this Router and AutoLoader - the router I did for a SO question, so it's less robust then the autoloader. But it loads based on the Class by instantiating instead of building a path. The autoloader has fair documentation, and its simple
$endgroup$
– ArtisticPhoenix
3 hours ago
$begingroup$
Downvote reason please? This is "Code Review" and as a beginner I simply hoped for a professional to kindly take a look at this if possible :(
$endgroup$
– J. Doe
3 hours ago
$begingroup$
Downvote reason please? This is "Code Review" and as a beginner I simply hoped for a professional to kindly take a look at this if possible :(
$endgroup$
– J. Doe
3 hours ago
$begingroup$
$path is undefined in your question's code. Which means file_exists($path) is always false in this example. And if it made it far enough this would fail too require_once($path);$endgroup$
– ArtisticPhoenix
3 hours ago
$begingroup$
$path is undefined in your question's code. Which means file_exists($path) is always false in this example. And if it made it far enough this would fail too require_once($path);$endgroup$
– ArtisticPhoenix
3 hours ago
$begingroup$
@ArtisticPhoenix I simplified variable/constant names for here, it was a typo, thanks for pointing that out
$endgroup$
– J. Doe
3 hours ago
$begingroup$
@ArtisticPhoenix I simplified variable/constant names for here, it was a typo, thanks for pointing that out
$endgroup$
– J. Doe
3 hours ago
$begingroup$
That was what I figured. - no problem. Otherwise it looks ok, you may want to name our constants something more "verbose" so they don't cause any conflicts. I would probably do this as a constant
$controller = 'default'; then if(!defined('DEFAULT_CONTROLER')) define('DEFAULT_CONTROLER', 'default') That way if you assign it before including this you can override it. But thats not a big deal.$endgroup$
– ArtisticPhoenix
3 hours ago
$begingroup$
That was what I figured. - no problem. Otherwise it looks ok, you may want to name our constants something more "verbose" so they don't cause any conflicts. I would probably do this as a constant
$controller = 'default'; then if(!defined('DEFAULT_CONTROLER')) define('DEFAULT_CONTROLER', 'default') That way if you assign it before including this you can override it. But thats not a big deal.$endgroup$
– ArtisticPhoenix
3 hours ago
$begingroup$
P.S.S. I'm not trying to go with MVC - You don't have to go full on MVC, the benefit is that with an auto loader you can avoid touching the path at all. So there is no possiblillity for Directory Transversal ../../../config.php etc. I have this Router and AutoLoader - the router I did for a SO question, so it's less robust then the autoloader. But it loads based on the Class by instantiating instead of building a path. The autoloader has fair documentation, and its simple$endgroup$
– ArtisticPhoenix
3 hours ago
$begingroup$
P.S.S. I'm not trying to go with MVC - You don't have to go full on MVC, the benefit is that with an auto loader you can avoid touching the path at all. So there is no possiblillity for Directory Transversal ../../../config.php etc. I have this Router and AutoLoader - the router I did for a SO question, so it's less robust then the autoloader. But it loads based on the Class by instantiating instead of building a path. The autoloader has fair documentation, and its simple$endgroup$
– ArtisticPhoenix
3 hours ago
|
show 2 more comments
0
active
oldest
votes
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
J. Doe is a new contributor. Be nice, and check out our Code of Conduct.
draft saved
draft discarded
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f215528%2fis-this-a-good-practice-to-load-controllers%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
0
active
oldest
votes
0
active
oldest
votes
active
oldest
votes
active
oldest
votes
J. Doe is a new contributor. Be nice, and check out our Code of Conduct.
draft saved
draft discarded
J. Doe is a new contributor. Be nice, and check out our Code of Conduct.
J. Doe is a new contributor. Be nice, and check out our Code of Conduct.
J. Doe is a new contributor. Be nice, and check out our Code of Conduct.
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
draft saved
draft discarded
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f215528%2fis-this-a-good-practice-to-load-controllers%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
$begingroup$
Downvote reason please? This is "Code Review" and as a beginner I simply hoped for a professional to kindly take a look at this if possible :(
$endgroup$
– J. Doe
3 hours ago
$begingroup$
$pathis undefined in your question's code. Which meansfile_exists($path)is always false in this example. And if it made it far enough this would fail toorequire_once($path);$endgroup$
– ArtisticPhoenix
3 hours ago
$begingroup$
@ArtisticPhoenix I simplified variable/constant names for here, it was a typo, thanks for pointing that out
$endgroup$
– J. Doe
3 hours ago
$begingroup$
That was what I figured. - no problem. Otherwise it looks ok, you may want to name our constants something more "verbose" so they don't cause any conflicts. I would probably do this as a constant
$controller = 'default';thenif(!defined('DEFAULT_CONTROLER')) define('DEFAULT_CONTROLER', 'default')That way if you assign it before including this you can override it. But thats not a big deal.$endgroup$
– ArtisticPhoenix
3 hours ago
$begingroup$
P.S.S. I'm not trying to go with MVC- You don't have to go full on MVC, the benefit is that with an auto loader you can avoid touching the path at all. So there is no possiblillity for Directory Transversal../../../config.phpetc. I have this Router and AutoLoader - the router I did for a SO question, so it's less robust then the autoloader. But it loads based on the Class by instantiating instead of building a path. The autoloader has fair documentation, and its simple$endgroup$
– ArtisticPhoenix
3 hours ago