Dingo Framework - ALPHA Testing Open! - Page 2

ETbyrne · 03-29-2009, 02:50 PM

Sure thing Kalle, consider it done... now.

Kalle · 03-30-2009, 05:10 PM

Quote:

Originally Posted by Tanax

Offtopic- How did you get @ php . net mail?

I contribute to abit of all on php.net, the php source, extensions, website, documentation and translations. Basiclly all except for PEAR & GTK.

Edit; I tried to browse the website a few times but it seems to timeout all the time :/

ETbyrne · 03-31-2009, 12:41 AM

Quote:

Edit; I tried to browse the website a few times but it seems to timeout all the time :/

Odd, it seems to be working just fine for me... Anyone else having trouble with the site?

Btw I'm in the process of uploading a new version of the framework. Same thing, just with a few fixes.

EDIT: New version uploaded

Kalle · 03-31-2009, 05:59 AM

I notices alot of things, alot of neatpicking but some could really be improved:

You are using double quotes for many things that doesn't use string interpolation, this is slower because php will seek through the string to find replacables, therefore use single quotes
PHP Code:

@define("PATH_INFO",$_SERVER['PATH_INFO']);

define will *not* emit an error, plus many SAPI's will not define PATH_INFO (for example Apache2 must have AcceptPathInfo = On), I know you have a note above saying so but perhaps find a more cross platform solution if possible
PHP Code:

$url = $route["$url"];

Related to the comment about string interpolation, you don't need php to spend extra time searching the string to replace one value you don't concate with something else, so simply use:

PHP Code:

$url = $route[$url];
PHP Code:

if(!empty($route[preg_replace('/^(\/)/','',$url)]))

without testing it, it looks like this would emit an E_NOTICE because the value may not exist in the $route array
PHP Code:

if(is_array(!empty($plugin)))

This expression is not logical value, the value you pass to is_array() is the value of an expression meaning the value is a boolean and therefore it can never be an array
PHP Code:

$x += 1;

, you can use pre/post fixes here, not that it plays a big deal:

PHP Code:

$x++;
PHP Code:

eval("\$_controller->{$url[1]}($arguments);");

this can be very very dangerous, php already supports variable functions, functors and so on, use the call_user_func[_array] functions and don't build arguments in a string but rather an array and run it like:

PHP Code:

call_user_func_array(Array($_controller, $url[1]), $arguments);

That was just a few by reading over the boostrap, I would also use __autoload()/spl_autoload_register() to define an autoloader and load classes only as they are needed. Secondly I would register a default exception handler and then throw exceptions instead of calling $dingo->error() all the time, and then make the exception handler format the error type depending on the exception, SPL offers alot of standard exceptions you can use and they can be found here.

ps. your changelog is a .ini, but its not a real ini file?

Edit; whops, seems like I came to add a bogus entry :)

ETbyrne · 03-31-2009, 04:57 PM

Quote:

define will *not* emit an error, plus many SAPI's will not define PATH_INFO (for example Apache2 must have AcceptPathInfo = On)

The '@' is there to prevent an error from $_SERER['PATH_INFO'] which, oddly, sometimes gives an error even whe it is present.

PHP Code:


			
if(!empty($route[preg_replace('/^(\/)/','',$url)]))

Does not produce an E_NOTICE error.

PHP Code:


			
eval("\$_controller->{$url[1]}($arguments);");

Quote:

this can be very very dangerous, php already supports variable functions, functors and so on, use the call_user_func[_array] functions and don't build arguments in a string but rather an array and run it like:

PHP Code:


			
call_user_func_array(Array($_controller, $url[1]), $arguments);

Thanks, I'll try that

Quote:

ps. your changelog is a .ini, but its not a real ini file?

Lol, actually I did that because it is easier to read in my text editor. I should probably change that! I'll also see about little performance enhancements like changing double quotes to single quotes.

So, any thoughts on the the framework from a application developers perspective? Like, how controllers work, how you access views, ect...

Salathe · 03-31-2009, 06:06 PM

Quote:

Originally Posted by ETbyrne

The '@' is there to prevent an error from $_SERER['PATH_INFO'] which, oddly, sometimes gives an error even whe it is present.

There should never be an error if it is present. However, if the value is not present a NOTICE will be raised and the define will assign NULL. If you are getting an error, is it repeatable and under what circumstances does it occur?

Quote:

Originally Posted by ETbyrne

PHP Code:


			
if(!empty($route[preg_replace('/^(\/)/','',$url)]))

Does not produce an E_NOTICE error.

Like isset, calling empty will not raise any NOTICE if the variable is not found.

Quote:

Originally Posted by ETbyrne

So, any thoughts on the the framework from a application developers perspective? Like, how controllers work, how you access views, ect...

I've only had a very brief look at the framework, mostly as a result of Kalle's post. Your method of gathering the request variables and pushing them through to a controller is odd (constructing a string for the arguments and evaling it.

Also, perhaps I'm missing something, but the only way to have any form of dynamic routes is you use what you call plugins? For example, how could one have "user/1" refer to (user.php) controller::index(1) or something more abstracted like "img/abc.jpg" refer to (images.php) controller::stream('abc', 'jpg')? The Routes documentation doesn't make any mention of any more basic than straight substitution (mypage => mycontroller/func/argument).

Your requirement of loading MySQLi should be disabled by default. Not every application wants to always have access to a database and even if they did, not necessarily MySQL via the MySQLi extension.

Helpers: I'd like to see these namespaced (class-based... we can use "real" namespaces somewhere in the future). For example, you have the functions base_url, page_url, model_url and redirect; why not have these as url::base, url::page, url::model and url::redirect? The same can be said for cookie::set and cookie::delete (no cookie::get?).

You use short PHP tags in places, please go through and replace them will their full alternatives. You can't expect short_tags to be turned on by default, nor for people to turn them on just to support your framework.

Models: accessible by the URL? I'm not sure this should be the case, or even if it is they should always be routed through a router.

I think your whole section of index.php under "Detect Illegal Characters in URL" could do with some re-working. How you construct the regular expression doesn't result in what I think you think it does (confused yet?).

That's all, after a very quick look. I do look forward to having a really good delve into the whole framework soon though.

ETbyrne · 03-31-2009, 07:22 PM

Quote:

Like isset, calling empty will not raise any NOTICE if the variable is not found.

Which is exactly what I want it to do, not raise a notice.

Quote:

Also, perhaps I'm missing something, but the only way to have any form of dynamic routes is you use what you call plugins? For example, how could one have "user/1" refer to (user.php) controller::index(1) or something more abstracted like "img/abc.jpg" refer to (images.php) controller::stream('abc', 'jpg')? The Routes documentation doesn't make any mention of any more basic than straight substitution (mypage => mycontroller/func/argument).

At the moment only static routes are supported. I am planning on adding dynamic route support in the near future, preferably before the first BETA release. For the time being I'm just using Mod_Rewrite.

A dynamic route in the future might look like this:

PHP Code:


			
$route['mypage/([0-9]+)'] = 'controller/function/arg1/$1';

Quote:

Helpers: I'd like to see these namespaced (class-based... we can use "real" namespaces somewhere in the future). For example, you have the functions base_url, page_url, model_url and redirect; why not have these as url::base, url::page, url::model and url::redirect? The same can be said for cookie::set and cookie::delete (no cookie::get?).

Good idea. Probably should, and will, be done. A cookie::get() function is not necessary because you can use the built in $this->dingo->cookie() function.

Quote:

Models: accessible by the URL? I'm not sure this should be the case, or even if it is they should always be routed through a router.

One of my big pet peeves with other frameworks was the fact that you had to load models from a controller. To me that kinda defeated the whole purpose of having a model in the first place. In Dingo controllers display pages and models make changes to the database/filesystem/whatever.

Separating the models from the controllers gives your URL scheme a lot more freedom. For example: let's say you have a page users/edit/[num] to edit a user. You can have the form for that page submit to the model index.php?model=users/edit/[num], instead of having to create another controller with a different URL like users/edit2/[num] to manipulate the database.

Confusing?

Quote:

I think your whole section of index.php under "Detect Illegal Characters in URL" could do with some re-working. How you construct the regular expression doesn't result in what I think you think it does (confused yet?).

I'll have to revisit that, all I remember is it being a total pain in the you know what!

Btw, I'm considering restructuring the bootstrap for better modularity. If that makes any sense.

Thanks for all the detailed feedback everyone!

Salathe · 03-31-2009, 08:21 PM

Quote:

Originally Posted by ETbyrne

Confusing?

Yes. What about things which shouldn't be in the model but might be necessary when invoking an editing page? Authorising the current user, validation of the supplied input data, routing different scenarios through to different views, etc.. Is that all to be done in the model?

Very good to hear that you're planning on allowing more dynamic routing! cookie::get is only a helper, it can call Dingo::cookie if you really want it to. Without it, the API just seems unbalanced.

ETbyrne · 03-31-2009, 08:31 PM

Quote:

Yes. What about things which shouldn't be in the model but might be necessary when invoking an editing page? Authorising the current user, validation of the supplied input data, routing different scenarios through to different views, etc.. Is that all to be done in the model?

I usually use auto-loaded helpers for repetitious tasks like authorizing the current user, validations, ect...

Quote:

Very good to hear that you're planning on allowing more dynamic routing! cookie::get is only a helper, it can call Dingo::cookie if you really want it to. Without it, the API just seems unbalanced.

I suppose I could, it can't really hurt.

I was just looking at namespaces in PHP 5.3 and was very, very, very impressed! I think I'll be restructuring the framework to use them sometime soon! I want to change it so that libraries use namespaces instead of loading a class into the $this->dingo object. For example running a MySQLi query would look like this:

Code:

dingo\mysqli->query($sql);

Instead of

PHP Code:


			
$this->dingo->mysqli->query($sql);

I think that's right... Btw I used the CODE BBcode block for the first piece of code because the PHP one doesn't like my backslash...

ETbyrne · 04-01-2009, 01:25 AM

I just uploaded a new version to the site with a few fixes, most of which where outlined by Kalle.

Salathe · 04-01-2009, 11:25 AM

Any chance of using SVN/Git so we can just suck down any changes without having to re-download a zip file from your site? It'll help us (and you, perhaps) keep up-to-date with what exactly changed and when.

sketchMedia · 04-01-2009, 12:20 PM

I'm in favor of that (SVN that is, don't know Git), xp-dev have free private svn repos you can use.

http://xp-dev.com

Not had chance to properly look at this yet, been too busy

, however my initial concerns have been mentioned (use of eval being one of the primary ones).
I'll research further before I make any recommendations etc.

ETbyrne · 04-01-2009, 03:35 PM

Quote:

however my initial concerns have been mentioned (use of eval being one of the primary ones).

I removed eval() completely from the framework with the latest update. I'll think about putting up an SVN...

ETbyrne · 04-01-2009, 04:51 PM

I might setup a code repository on Github later today...

Kalle · 04-02-2009, 09:57 AM

or you can be old school like us at php.net and use CVS :)

ETbyrne · 04-02-2009, 10:15 PM

I've been in the process of updating my local server to PHP 5.3 so I can start restructuring Dingo to use namespaces. I'm also working on replacing PATH_INFO with REQUEST_URI or PHP_SELF.

sketchMedia · 04-03-2009, 11:34 AM

Quote:

or you can be old school like us at php.net and use CVS :)

Noooooo!!!! xD

ETbyrne · 04-05-2009, 02:53 PM

I've gotten a TON of work done on Dingo, enough to warrant changing it from version 0.1.2 to 0.1.3! I have the new version uploaded to the site. Here's a short list of what's changed:

Functions
They take a lot less writing. For example instead of doing this:

PHP Code:


			
$this->dingo->mysqli->query();

You can do this:

PHP Code:


			
$this->mysqli->query();

Organization
Helpers and libraries are now loaded from the "load" class:

PHP Code:


			
$this->load->helper();
$this->load->library();

Input data can now be accessed through the "input" class:

PHP Code:


			
$this->input->post();
$this->input->get();
$this->input->cookie();
$this->input->request();

URL Helper
The URL Helper functions are now placed in a class and can be accessed like so:

PHP Code:


			
url::base();
url::page();
url::model();
url::plugin_page();
url::plugin_model();
url::set_cookie();
url::delete_cookie();
url::redirect();

Routes
Routes can now use regular expressions:

PHP Code:


			
$route['one/([a-zA-Z]+)/([0-9]+)'] = 'query/$1/$2';

I'm in the process of updating the documentation on the site for the new version.

ETbyrne · 04-05-2009, 07:13 PM

I just uploaded a new one that fixed a couple bugs causing plugins not to work. If you downloaded the one I uploaded earlier today then download the new one and just replace index.php and the URL Helper with the new ones.

Btw happy Palm Sunday!

WebSavvy · 04-06-2009, 06:46 PM

Looks like you've put an awful lot of work into this. I wish you great success with it.

Happy Palm Sunday to you, too.