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('
%s

', __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

Comments

hinikato’s picture

Issue summary: View changes

Fixed typo in the PHP code.

natemow’s picture

Status: Needs review » Needs work
FileSize
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.

natemow’s picture

Issue summary: View changes

Fixed Git link

hinikato’s picture

Issue summary: View changes

Synchronized with README.txt file.

hinikato’s picture

Issue summary: View changes

Updated links

hinikato’s picture

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

hinikato’s picture

Status: Needs work » Needs review

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

patrickd’s picture

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

hinikato’s picture

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

hinikato’s picture

Status: Needs work » Needs review
drupaledmonk’s picture

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.

hinikato’s picture

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.

hinikato’s picture

Issue summary: View changes

Updated text

klausi’s picture

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.
hinikato’s picture

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.

hinikato’s picture

Status: Needs work » Reviewed & tested by the community

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

klausi’s picture

Status: Reviewed & tested by the community » Needs review

Don't RTBC your own issues.

hinikato’s picture

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.

klausi’s picture

Status: Reviewed & tested by the community » Needs review

Because I have not found the time to review your code again. Someone other than you needs to verify that the changes have been made and that it is indeed ready now.

hinikato’s picture

okay, please review it someone.

hinikato’s picture

Issue summary: View changes

Fixed typo

hinikato’s picture

Issue summary: View changes

Sync with README.txt

hinikato’s picture

Issue summary: View changes

Added more descriptions.

hinikato’s picture

Issue summary: View changes

Sync with README.txt

hinikato’s picture

Issue summary: View changes

Added links on sandbox and git repo.

hinikato’s picture

Issue summary: View changes

Added how to clone description.

hinikato’s picture

Issue summary: View changes

Fixed git clone command.

scarer’s picture

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.

hinikato’s picture

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.

hinikato’s picture

Status: Needs work » Needs review
prashantgoel’s picture

Status: Needs review » Needs work

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

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.

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

patrickd’s picture

Status: Needs work » Needs review

this is no real issue - it's conceptual

please understand the module topic before just pasting automated reviews in

itsekhmistro’s picture

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.

hinikato’s picture

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:

  1. 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.
  2. Now hook_menu() may not be defined at all - all mapping can be defined in the your controllers.
  3. 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.

itsekhmistro’s picture

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.

klausi’s picture

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.

klausi’s picture

Issue summary: View changes

Added git clone command for the 7.x-1.x branch

hinikato’s picture

Issue summary: View changes

Added full project link.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Fixed typo.