Controller [#1389042]

Update: this project has moved to full drupal project!

http://drupal.org/project/controller - new project location.

Thanks for all who reviewed it!

Description

Controller is module that enables Page Controller pattern for your pages.
Page Controller is just class that have set of actions. Each action is a
separate method that will be called for specified path.

For example if you have the 'foo/bar' path (URI), then you can create a new
class called FooBarPageController and myAction method (you can use other names
of course). This method will be called for the 'http://host/foo/bar' address.

Example

Let's say that you have the 'mymodule' module and you want to call the
FooBarPageController::myAction() method for the 'foo/bar' path. To do that
do the following:
1. Create new directory mymodule/controllers/.

2. Create new file called FooBarPageController.php inside this directory.

3. Add the following code into it:

/**
 * My test controller class.
 */
class MymoduleFooBarPageController {
  /**
   * This method that will be called for the 'foo/bar' path.
   *
   * @path => 'foo/bar',
   * @access arguments => array('access arguments'),
   * @title => 'My test page',
   */
  function myAction() {
    return sprintf('<pre>%s</pre>', __METHOD__ . ' called with args: '
      . print_r(func_get_args(), 1));
  }
}

Please note your attention on the following concepts:
- The module name is 'mymodule' and the Controller class should be prefixed
with the CamelCased version of the module name with capital first latter. For
our example it is 'Mymodule' prefix. If the 'controller_use_ns' variable is on
(that you can change on the '/admin/settings/controller' page) the prefix will
be used as namespace, so the the full class name will be
\Mymodule\FooBarPageController.
- In the annotations (PHP comments) for the myAction() method we defined items
like @path, @access arguments and the @title. You can define other items that
can be defined in the hook_menu() and that described here:
http://api.drupal.org/api/drupal/developer!hooks!core.php/function/hook_...
It is the @title callback, @title arguments, @description, @page arguments etc.
There is the only one new item @path that inroduced by the 'controller' module.
The value of the each item is PHP code that can optionally end with the ; or
, character (it may be useful if you have a habit to enter them inside your
PHP code).

4. Define the hook_controller_api() as follows:

/**
 * Implements hook_controller_api().
 */
function mymodule_controller_api() {
  return '2.x';
}

5. After that clear the menu cache and navigate the 'foo/bar' path in your
browser. Then you will see that the MymoduleFooBarPageController::myAction()
will be called.

6. Format of the annotations is very simple:
just not add the quotes ('') for key and add the @ before it. For example the:

'page arguments' => array('foo' => 'bar'),

may be rewritten with PHP comments as follows:

/**
 * @page arguments => array('foo' => 'bar'),
 */

Each line may optionally end with the semicolon (;) or comma (,).

Other way to define your controllers

There is an other way that can be used to define your controllers. For example
above the FooBarPageController::myAction() method can be defined in the
hook_menu() as follows:

/**
 * @file
 * Your description...
 */

/**
 * Your implementation of the hook_menu();
 */
function mymodule_menu() {
  $items = array();
  $items['foo/bar'] = array(
    'action' => 'FooBarPageController::myAction',
    'access arguments' => array('access content'),
    'type' => MENU_CALLBACK,
  );
  return $items;
}

In this definition please note on the new 'action' key. It is a special item
that will be handled by the controller module. There is no 'path' item here
because the path defined as array key.

How it works?

In case of defining the menu items through annotations the 'controller'
module will collect and parse the annotations and rewrite them with using
of the new 'action' item. After that the 'controller' module will expand
the 'action' definition into other item that follows Drupal's conventions.
For example the following definition:

/**
 * Your implementation of the hook_menu();
 */
function mymodule_menu() {
  $items = array();
  $items['foo/bar'] = array(
    'action' => 'FooBarPageController::myAction',
    'access arguments' => array('access content'),
    'type' => MENU_CALLBACK,
  );
  return $items;
}

will be expanded to the following definition:

/**
 * Your implementation of the hook_menu();
 */
function mymodule_menu() {
  $items = array();
  $my_file_path = drupal_get_path('module', 'mymodule')
    . '/controllers/FooBarPageController.php';
  $items['foo/bar'] = array(
    'page callback' => 'controller_menu_handler',
    'page arguments' => array('MymoduleFooBarPageController::myAction'),
    'file' => $my_file_path,
    'access arguments' => array('access content'),
    'type' => MENU_CALLBACK,
  );
  return $items;
}

When the 'foo/bar' path will be navigated the 'controller_menu_handler'
function will be called when in turns will create an instance of the
MymoduleFooBarPageController and will call the myAction() method.

Links

1. Sandbox link

Comment	File	Size	Author
#16	controller-branch7.x-1-08-03-2012.txt	981 bytes	scarer
#16	controller-branch6.x-2-08-03-2012.txt	981 bytes	scarer
#1	1389028_pareview.txt	2.5 KB	natemow

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #0.0

hinikato CreditAttribution: hinikato commented 30 December 2011 at 05:49

Issue summary:

View changes

Fixed typo in the PHP code.

Comment #1

natemow CreditAttribution: natemow commented 30 December 2011 at 13:24

Status:

Needs review

» Needs work

File	Size
1389028_pareview.txt	2.5 KB

Automated review attached -- This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.

Comment #1.0

natemow CreditAttribution: natemow commented 30 December 2011 at 13:24

Issue summary:

View changes

Fixed Git link

Comment #1.1

hinikato CreditAttribution: hinikato commented 31 December 2011 at 04:46

Issue summary:

View changes

Synchronized with README.txt file.

Comment #1.2

hinikato CreditAttribution: hinikato commented 31 December 2011 at 04:51

Issue summary:

View changes

Updated links

Comment #2

hinikato CreditAttribution: hinikato commented 31 December 2011 at 04:54

All errors/notes have been fixed. Please review the project again.

Comment #3

hinikato CreditAttribution: hinikato commented 31 December 2011 at 05:00

Status:

Needs work

» Needs review

All errors/notes have been fixed. Please review the project again.

Comment #4

patrickd CreditAttribution: patrickd commented 18 January 2012 at 17:14

Status:

Needs review

» Needs work

Still some left: http://ventral.org/pareview/httpgitdrupalorgsandboxhinikato1389028git

/**
 * Implements hook_help().
 */
function controller_help($path, $arg) {

}

If you don't provide help here - don't implement it.

The code that provides the main functionality (especially controller_menu_alter) have not a single inline comment.
We want to understand your code, we want to know why you made the decision for the implementation. Explain us the significant codelines. There is no "too much" commenting, the more comments, the better we know how and why you're coding the way you do.
Have a look at some core modules and also the core its self, this is the way we love it and the way it should be ;-)

Further reviews will follow after you enhanced your code documentation.

regards

Comment #5

hinikato CreditAttribution: hinikato commented 18 January 2012 at 20:31

Okay, I added inline comments and all errors were fixed:
http://ventral.org/pareview/httpgitdrupalorgsandboxhinikato1389028git-6x-1x

Comment #6

hinikato CreditAttribution: hinikato commented 18 January 2012 at 20:36

Status:

Needs work

» Needs review

Comment #7

drupaledmonk CreditAttribution: drupaledmonk commented 21 January 2012 at 18:51

Status:

Needs review

» Reviewed & tested by the community

Hey, I am new to this reviewing process. I have given a manual look to your code and it looks /works fine,

This is an open question as I am not sure on this:
Is it not recommended to include this [sandbox/hinikato/1389028.git] / forms / settings_form.php in an admin file or .module

Do we not generally include stuff with a .inc extension.

Comment #8

hinikato CreditAttribution: hinikato commented 21 January 2012 at 23:43

Hi, f4k1r,

Okay, thank you for you feedback. I see to progress of Drupal 8 and I see that it uses *.php files for classes. An any controller is class so I think it will be not a problem to use the *.php extension for the files inside the controllers/ directory. According forms/ php directory I think it can be useful alternative because:

Form can be easily located by file name
All forms will be in one place that is forms/ directory
All forms will follow one convention to avoid cluttering of the other files

I think the settings_form.php file can be renamed to settings_form.inc file to follow general Drupal conventions for included files that don't contain classes.

Comment #8.0

hinikato CreditAttribution: hinikato commented 21 January 2012 at 23:43

Issue summary:

View changes

Updated text

Comment #9

klausi

he/him||they/them

German

🇦🇹 Vienna

CreditAttribution: klausi commented 22 January 2012 at 23:24

Status:

Reviewed & tested by the community

» Needs work

Review of the 6.x-1.x branch:

Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards).


FILE: ...drupal-7/sites/all/modules/pareview_temp/test_candidate/controller.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 1 | ERROR | "description" property is missing in the info file
--------------------------------------------------------------------------------

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review:

"'access arguments' => array('access administration pages'),": this permissions is too generic, you should provide your own.
"hook_controller_no_ns": your module provides a hook, so there should be a controller.api.php file that describes how the hook should be used.
"array_key_exists(CONTROLLER_MENU_ACTION_KEY, $item)": isset($item[CONTROLLER_MENU_ACTION_KEY]) is shorter and more common in the Drupal world.
controller_settings_form(): all user facing text should run through t() for translation.
ControllerSettingsController is a bad example because it just adds one layer of indirection. You should specify drupal_get_form directly as page callback in hook_menu() and controller_settings_form as page argument.

Comment #10

hinikato CreditAttribution: hinikato commented 23 January 2012 at 00:18

Hi, klausi

I agreed with most of your notes. I think the ControllerSettingsController is should be left as is - with addition of documentation it is the example of using that will help folks to understand how the 'controller' works more quickly.

Thank you for your feedback.

Comment #11

hinikato CreditAttribution: hinikato commented 23 January 2012 at 00:41

Status:

Needs work

» Reviewed & tested by the community

I fixed all found bugs/errors and return the previous status for this issue.

Comment #12

klausi

he/him||they/them

German

🇦🇹 Vienna

CreditAttribution: klausi commented 23 January 2012 at 09:02

Status:

Reviewed & tested by the community

» Needs review

Don't RTBC your own issues.

Comment #13

hinikato CreditAttribution: hinikato commented 23 January 2012 at 17:28

Status:

Needs review

» Reviewed & tested by the community

klausi, the previous status was RTBC, and this status was set by f4k1r. I fixed all found by me, other peoples and you bugs. Why you don't changed the status back to RTBC? It seems like it is your error.

Comment #14

klausi

he/him||they/them

German

🇦🇹 Vienna

CreditAttribution: klausi commented 23 January 2012 at 23:03

Status:

Reviewed & tested by the community

» Needs review

scarer CreditAttribution: scarer commented 8 March 2012 at 04:16

File	Size
controller-branch6.x-2-08-03-2012.txt	981 bytes
controller-branch7.x-1-08-03-2012.txt	981 bytes

There are still some errors remaining.

The first is a warning to make sure that the PHP input code is being checked OK. (Line 139 in MenuItemsCollector.php). There's a guide to using drupal_eval safely here: http://drupal.org/node/715010 listed in the writing secure code section.

The second warning advises to use "protected" instead of "private" for your methods - found in file NamingService.php on line 8.

The same errors are appearing for both branches (6.x-2 and 7.x-1). Please see attached output from ventral.

Review bonus.

Comment #17

hinikato CreditAttribution: hinikato commented 8 March 2012 at 07:16

Status:

Needs review

» Needs work

I fixed the private/protected issue. The using of drupal_eval() is required to properly module work.
No additional changes required, because it is not a security hole - user defines hook_menu() rules in the PHP comments and they are parsed by module after that they are evaluated.

Comment #18

hinikato CreditAttribution: hinikato commented 8 March 2012 at 07:18

Status:

Needs work

» Needs review

Comment #19

prashantgoel CreditAttribution: prashantgoel commented 8 April 2012 at 16:55

Status:

Needs review

» Needs work

Review of the 7.x-1.x branch:

Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.

Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/./test_candidate/services/MenuItemsCollector.php:
 +139: [critical] Using eval() or drupal_eval() in your module's code could have a security risk if the PHP input provided to the function contains malicious code.

Status Messages:
 Coder found 11 projects, 11 files, 1 critical warnings, 0 warnings were flagged to be ignored

Source: http://ventral.org/pareview - PAReview.sh online service

Comment #20

patrickd CreditAttribution: patrickd commented 8 April 2012 at 20:49

Status:

Needs work

» Needs review

this is no real issue - it's conceptual

please understand the module topic before just pasting automated reviews in

Comment #21

itsekhmistro CreditAttribution: itsekhmistro commented 9 April 2012 at 09:29

Hi, I manually reviewed the module.

Could you please just answer these two questions:

curently, It's not possible to pass own access_callback for each Action of the controller, is it?
Is it possible to apply some urlaliases to controller/action callbacks?

I don't find any blocking issues in the code. But have to mentioned that imho, it's concept is not drupal driven, and the only advantage it will give - is easy adding new forms/functionality for none drupal-coders (I mean "new Drupal developers").

I believe this module has rights to exist.

At the moment, the only suggest is - I think it make sense to add an Exception handler into function controller_menu_handler().
Thank you.

Comment #22

hinikato CreditAttribution: hinikato commented 9 April 2012 at 11:14

Hi, itsekhmistro

Thank you for that review, it is very helpful! I will try to answer on your questions below.

curently, It's not possible to pass own access_callback for each Action of the controller, is it?

Short answer: no exactly, you can use the 'access callback' item to define your access functions.
More descriptive answer: theoretically, you can set any item that defined in the hook_menu() documentation, except the 'page callback', 'file' and the 'file path' items that will be overwritten by the 'controller' module.

Is it possible to apply some urlaliases to controller/action callbacks?

I thought about aliases too. So this feature can be considered for future releases.

I don't find any blocking issues in the code. But have to mentioned that imho, it's concept is not drupal driven, and the only advantage it will give - is easy adding new forms/functionality for none drupal-coders (I mean "new Drupal developers").

There are some additional features yet:

Each Controller is class, so you all OOP goodies can be applied, for example you can define new base Controller class and extend it in your controllers.
Now hook_menu() may not be defined at all - all mapping can be defined in the your controllers.
Your menu/path handlers can be placed within one controllers/ directory and grouped with using of classes. So you can more easily find your menu handler by looking on name of class/file.

At the moment, the only suggest is - I think it make sense to add an Exception handler into function controller_menu_handler().

It may be good idea and can be considered for future releases too.

Thanks.

Comment #23

itsekhmistro CreditAttribution: itsekhmistro commented 10 April 2012 at 16:58

Status:

Needs review

» Reviewed & tested by the community

Thank you for the provided answers.

It works fine for me, I think it is ready for public.

Comment #24

klausi

he/him||they/them

German

🇦🇹 Vienna

CreditAttribution: klausi commented 13 April 2012 at 19:16

Status:

Reviewed & tested by the community

» Fixed

You have not listed any reviews of other project applications in your issue summary as strongly recommended here: http://drupal.org/node/1011698

manual review (7.x-1.x):

"php = 5.2": drupal 7 already requires PHP 5.2, so this is useless.
controller_include_class(): that function does not make sense in Drupal 7 as all classes of all modules have to be registered in the info files.

But otherwise this looks good.

Thanks for your contribution, hinikato! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.