Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Single Task Controllers RFC #3503

Closed
wants to merge 22 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
45eb8d6
Merge pull request #2 from joomla/master
Mathewlenning Sep 18, 2013
2be4e10
Added Single Task Controllers to the CMS Directory
Mathewlenning Apr 25, 2014
9f7a6d3
Removed the cms/model and cms/view directories. I'm going to create
Mathewlenning Apr 25, 2014
8f149bb
Change the class name of cms/controller/update/close.php to fit the
Mathewlenning Apr 25, 2014
a308d43
Updated cms/controller/add.php & cms/controller/edit.php to return the
Mathewlenning Apr 25, 2014
2addb89
Renamed controllers to work with naming convention.
Mathewlenning Apr 26, 2014
bbb3330
slight changes
Mathewlenning Apr 30, 2014
5da5e13
Merge pull request #6 from Mathewlenning/revised
Mathewlenning Apr 30, 2014
83fc12c
Adjusted the alignment on the @subpackage tag
Mathewlenning May 3, 2014
12426a6
Fixed typo in JControllerCreateBase. Thanks Herman
Mathewlenning May 4, 2014
4654fd6
Added the models to the cms dir
Mathewlenning May 4, 2014
ab223dd
Codesniffer changes and refactoring of getModel to remove duplicate c…
Mathewlenning May 5, 2014
486546b
Refactoring task abort redirections
Mathewlenning May 6, 2014
9f5c697
Reformatted to fit Joomla! standards
Mathewlenning May 6, 2014
eb88f74
More reformatted
Mathewlenning May 6, 2014
a734bef
Added JControllerCms::internalRedirect, JControllerCms::mergeModels, …
Mathewlenning May 6, 2014
8a65be7
Renamed JControllerCms::internalRedirect to JControllerCms::executeIn…
Mathewlenning May 6, 2014
00c442f
Added Ajax support for publish & unpublish.
Mathewlenning May 7, 2014
3c9d4e8
Cleaning up the models. Changed JModelData::getTable and createTable …
Mathewlenning May 8, 2014
cfd10cf
Added the views
Mathewlenning May 8, 2014
5176d24
Added the table. This is still extending JTable, but it was written u…
Mathewlenning May 8, 2014
a443c47
Fixed JViewCms::setModel call in JControllerDisplay::execute method
Mathewlenning May 9, 2014
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions libraries/cms/controller/add.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<?php
/**
* @package Joomla.Libraries
* @subpackage Captcha
*
* @copyright Copyright (C) 2005 - 2013 Open Source Matters, Inc. All rights reserved.
* @license GNU General Public License version 2 or later; see LICENSE.txt
*/

defined('JPATH_PLATFORM') or die;

class JCmsControllerAdd extends JCmsControllerDisplay
{
/**
* Instantiate the controller.
*
* @param JInput $input The input object.
* @param JApplicationBase $app The application object.
* @param array $config Configuration
* @since 12.1
*/
public function __construct(JInput $input, $app = null, $config = array())
{
$input->set('layout', 'edit');

parent::__construct($input, $app, $config);
}

/**
* (non-PHPdoc)
* @see JCmsControllerDisplay::execute()
*/
public function execute()
{
$config = $this->config;
$prefix = $this->getPrefix();
$model = $this->getModel($prefix, $config['subject'], $config);

if (!$model->allowAction('core.create'))
{
$msg = $this->translate('JLIB_APPLICATION_ERROR_CREATE_RECORD_NOT_PERMITTED');
$url = 'index.php?option='.$config['option'].'&task=display.'.$config['subject'];
$this->setRedirect($url, $msg, 'error');
return false;
}

parent::execute();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd much rather see us return parent::execute in situations like this - because that can return false or true as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I'll make the change when I wake up tomorrow.

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Apr 26, 2014, at 12:34 AM, George Wilson [email protected] wrote:

In libraries/cms/controller/add.php:

  • */
  • public function execute()
  • {
  •   $config = $this->config;
    
  •   $prefix = $this->getPrefix();
    
  •   $model = $this->getModel($prefix, $config['subject'], $config);
    
  •   if (!$model->allowAction('core.create'))
    
  •   {
    
  •       $msg = $this->translate('JLIB_APPLICATION_ERROR_CREATE_RECORD_NOT_PERMITTED');
    
  •       $url = 'index.php?option='.$config['option'].'&task=display.'.$config['subject'];
    
  •       $this->setRedirect($url, $msg, 'error');
    
  •       return false;
    
  •   }
    
  •   parent::execute();
    
    I'd much rather see us return parent::execute in situations like this - because that can return false or true as well


Reply to this email directly or view it on GitHub.

return true;
}

}
242 changes: 242 additions & 0 deletions libraries/cms/controller/base.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,242 @@
<?php
/**
* @package Joomla.Libraries
* @subpackage Captcha
*
* @copyright Copyright (C) 2005 - 2013 Open Source Matters, Inc. All rights reserved.
* @license GNU General Public License version 2 or later; see LICENSE.txt
*/

defined('JPATH_PLATFORM') or die;

abstract class JCmsControllerBase extends JControllerBase
{
/**
* Configuration variables
* @var array
*/
protected $config;

/**
* Associative array of models
* stored as $models[$prefix][$name] used by get models
* @var array
*/
protected $models = array();

/**
* URL for redirection.
*
* @var string
* @since 12.2
* @note Replaces _redirect.
*/
protected $redirect;

/**
* Redirect message.
*
* @var string
* @since 12.2
* @note Replaces _message.
*/
protected $message;

/**
* Redirect message type.
*
* @var string
* @since 12.2
* @note Replaces _messageType.
*/
protected $messageType;

/**
* Instantiate the controller.
*
* @param JInput $input The input object.
* @param JApplicationBase $app The application object.
* @param array $config Configuration
* @since 12.1
*/
public function __construct(JInput $input, $app = null, $config = array())
{
parent::__construct($input, $app);

if (!array_key_exists('option', $config))
{
$config['option'] = $input->get('option');
}

$this->config = $config;
}

/**
* Method to check the session token
* @return void
*/
protected function validateSession()
{
JSession::checkToken() or jexit(JText::_('JINVALID_TOKEN'));
}

/**
* Method to refresh the session token to prevent the back button
* @return void
*/
protected function refreshToken()
{
$session = JFactory::getSession();
$session->getToken(true);
}

/**
* Method to translate a string using JText::_() method
* @param string $string
* @return string translation result
*/
protected function translate($string)
{
return JText::_($string);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally not a fan of this - I'd much rather just see us use JText::_ directly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it doesn't make much sense to add a wrapper around JText.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this because personally I don't like static functions spread all over the place. Also I'm almost positive it is an anti-pattern.

This way if Joomla decides to scrap JText now or in the future there is only one function that has to change.

You'll probably notice with the exception of JFactory, I've wrapped all statics in class methods. And to be honest I regret not wrapping it too.

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Apr 26, 2014, at 12:44 AM, Thomas Hunziker [email protected] wrote:

In libraries/cms/controller/base.php:

  • * @return void
  • */
  • protected function refreshToken()
  • {
  •   $session = JFactory::getSession();
    
  •   $session->getToken(true);
    
  • }
  • /**
  • * Method to translate a string using JText::_() method
  • * @param string $string
  • * @return string translation result
  • */
  • protected function translate($string)
  • {
  •   return JText::_($string);
    
    I agree that it doesn't make much sense to add a wrapper around JText.


Reply to this email directly or view it on GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this because personally I don't like static functions spread all over the place.

I'd say the uses in the controllers are out least concern here. If we ever are going to change JText::_(), we will have to change it in many many places. Let's do a proper replacement when JText is deprecated and not introduce a new method in a PR which has a completely different target. Personally I don't think that will happen anytime soon.
As for the other wrappers (I think you mean the validateSession and refreshToken methods), I wouldn't do that neither. IMHO it just complicates the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I would anything touching JSession just because we know the
framework team have decided to use symfony session package so I guess the
CMS might transition too in the not too distant future. So abstracting that
out is a good idea in my opinion

On Friday, April 25, 2014, Thomas Hunziker [email protected] wrote:

In libraries/cms/controller/base.php:

  • * @return void
  • */
  • protected function refreshToken()
  • {
  •   $session = JFactory::getSession();
    
  •   $session->getToken(true);
    
  • }
  • /**
  • * Method to translate a string using JText::_() method
  • * @param string $string
  • * @return string translation result
  • */
  • protected function translate($string)
  • {
  •   return JText::_($string);
    

I did this because personally I don't like static functions spread all
over the place.

I'd say the uses in the controllers are out least concern here. If we ever
are going to change JText::_(), we will have to change it in many many
places. Let's do a proper replacement when JText is deprecated and not
introduce a new method in a PR which has a completely different target.
Personally I don't think that will happen anytime soon.
As for the other wrappers (I think you mean the validateSession and
refreshToken methods), I wouldn't do that neither. IMHO it just complicates
the code.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3503/files#r12007010
.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because the Framework is doing something doesn't automatically mean
the CMS is doing it too...

On Friday, April 25, 2014, George Wilson [email protected] wrote:

In libraries/cms/controller/base.php:

  • * @return void
  • */
  • protected function refreshToken()
  • {
  •   $session = JFactory::getSession();
    
  •   $session->getToken(true);
    
  • }
  • /**
  • * Method to translate a string using JText::_() method
  • * @param string $string
  • * @return string translation result
  • */
  • protected function translate($string)
  • {
  •   return JText::_($string);
    

Actually I would anything touching JSession just because we know the
framework team have decided to use symfony session package so I guess the
CMS might transition too in the not too distant future. So abstracting that
out is a good idea in my opinion
… <#14599fe8ed5f5269_>
On Friday, April 25, 2014, Thomas Hunziker <[email protected]javascript:_e(%7B%7D,'cvml','[email protected]');>
wrote: In libraries/cms/controller/base.php: > + * @returnhttps://github.com/returnvoid > + / > + protected function refreshToken() > + { > + $session =
JFactory::getSession(); > + $session->getToken(true); > + } > + > + /
* > +

  • Method to translate a string using JText::() method > + * @paramhttps://github.com/paramstring $string > + *
    @return https://github.com/return string translation result > + */ > +
    protected function translate($string) > + { > + return JText::
    ($string); I
    did this because personally I don't like static functions spread all over
    the place. I'd say the uses in the controllers are out least concern here.
    If we ever are going to change JText::_(), we will have to change it in
    many many places. Let's do a proper replacement when JText is deprecated
    and not introduce a new method in a PR which has a completely different
    target. Personally I don't think that will happen anytime soon. As for the
    other wrappers (I think you mean the validateSession and refreshToken
    methods), I wouldn't do that neither. IMHO it just complicates the code. —
    Reply to this email directly or view it on GitHub<
    https://github.com/joomla/joomla-cms/pull/3503/files#r12007010> .


Reply to this email directly or view it on GitHubhttps://github.com//pull/3503/files#r12007289
.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't know yet what will happen and when. And it may well be that this methods are not suiteable for what we will do then and we have to deprecate them as well.
It may well be that we just use JSession as the wrapper and include Symfony or whatever transparent for the extension. There is no need to add a wrapper around the wrapper 😃

Static functions aren't evil by itself. There are perfectly fine useages of those.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good morning.

The truth is none of us know what is going to happen in the future, so any discussion about the future is nothing more than an assumption.

However if we choose to learn from the past which is absolute, we stand a better chance of improving our assumption. Correct me if I'm wrong, but as I recall we didn't used to use JSession::checkToken().

Forgive me for not recalling the exact class we used to use, but I remember the change because I had to go through my extension and change all the references.(which sucked)

@Bakual

Static functions aren't evil by itself. There are perfectly fine useages of those.

We are going to have to agree to disagree on this one.

As for the other wrappers (I think you mean the validateSession and refreshToken methods), I wouldn't do that neither. IMHO it just complicates the code.

For someone who has made it over the JLearning curve these might seem unnecessary, but when I code I like to assume the next person reading my code doesn't know any PHP. Which forces me to be very expressive.

Tell me which of these express the intention which is "To refresh the session token"

$session = JFactory::getSession();
$session->getToken(true);

or

$this->refreshToken();

Again which expresses the true intention which is "To make sure the current session is a valid"

JSession::checkToken() or jexit(JText::_('JINVALID_TOKEN'));

or

$this->validateSession();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tell me which of these express the intention which is "To refresh the session token"

Actually, I don't want to refresh an existing token. I just want to issue a new token. So $this->refreshToken() would be misleading and JFactory::getSession()->getToken(true); would be what I want.

JSession::checkToken() or jexit(JText::_('JINVALID_TOKEN')); does more than "make sure the current session is valid". It also ends the script if it's not. So the method name would be wrong anyway. If you want to make it correct you would have to use. The function to validate is JSession::checkToken() which describes exactly what it does. The jexit is already the fail condition of the failed validating.

It may come down to personal preferences of course. However I'm a big fan of keeping PRs on its topic and adding new methods to wrap static calls doesn't seem to be needed for adding single task contollers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refreshToken() doesn't return the token. It creates a new token and replaces the old token that is already in the session. This is why I call it refresh token.

I have to admit, I'm surprised that these simple functions that actually only serve the purpose of moving the responsibility for session checks into a reusable function (making building controllers easier) would be so heavily debated, while the fact that the inversion of the controller.task hasn't even gotten a mention.

Crazy world we live in.

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Apr 26, 2014, at 4:54 PM, Thomas Hunziker [email protected] wrote:

In libraries/cms/controller/base.php:

  • * @return void
  • */
  • protected function refreshToken()
  • {
  •   $session = JFactory::getSession();
    
  •   $session->getToken(true);
    
  • }
  • /**
  • * Method to translate a string using JText::_() method
  • * @param string $string
  • * @return string translation result
  • */
  • protected function translate($string)
  • {
  •   return JText::_($string);
    
    Tell me which of these express the intention which is "To refresh the session token"

Actually, I don't want to refresh an existing token. I just want to issue a new token. So $this->refreshToken() would be misleading and JFactory::getSession()->getToken(true); would be what I want.

JSession::checkToken() or jexit(JText::_('JINVALID_TOKEN')); does more than "make sure the current session is valid". It also ends the script if it's not. So the method name would be wrong anyway. If you want to make it correct you would have to use. The function to validate is JSession::checkToken() which describes exactly what it does. The jexit is already the fail condition of the failed validating.

It may come down to personal preferences of course. However I'm a big fan of keeping PRs on its topic and adding new methods to wrap static calls doesn't seem to be needed for adding single task contollers.


Reply to this email directly or view it on GitHub.

}

/**
* Method to get the option prefix from the input
* @return string ucfirst(substr($this->config['option'], 4));
*/
protected function getPrefix()
{
$prefix = ucfirst(substr($this->config['option'], 4));

return $prefix;
}

/**
* Method to get a model, creating it if it doesn't already exist.
* @param string $prefix
* @param string $name
* @param array $config
* @return JCmsModelBase
* @throws ErrorException
*/
public function getModel($prefix, $name, $config = array())
{
$prefix = ucfirst($prefix);
$name = ucfirst($name);

if (isset($this->models[$prefix][$name]))
{
return $this->models[$prefix][$name];
}

$class = $prefix.'Model'.$name;

if (!class_exists($class))
{
throw new ErrorException(JText::sprintf('JLIB_APPLICATION_ERROR_MODELCLASS_NOT_FOUND', $class));
return false;
}

$config = $this->normalizeConfig($config);

$this->models[$prefix][$name] = new $class($config);

return $this->models[$prefix][$name];
}

/**
* Method to insure all config variables are are included.
* Intended to be used in getModel, getView and other factory methods
* that can be passed a config array.
* @param array $config to normalize
*/
protected function normalizeConfig($config)
{
//Safe merge. will not overwrite existing keys
$config += $this->config;
}

/**
* Redirects the browser or returns false if no redirect is set.
* @return boolean False if no redirect exists.
*
* @since 12.2
*/
public function redirect()
{
if ($this->hasRedirect())
{
$app = $this->app;

// Enqueue the redirect message
$app->enqueueMessage($this->message, $this->messageType);

// Execute the redirect
$app->redirect($this->redirect);
}

return false;
}

/**
* Method to check if the controller has a redirect
* @return boolean
*/
public function hasRedirect()
{
if (!empty($this->redirect))
{
return true;
}
return false;
}

/**
* Set a URL for browser redirection.
*
* @param string $url URL to redirect to.
* @param string $msg Message to display on redirect. Optional.
* @param string $type Message type. Optional, defaults to 'message'.
* @param bool $useJRoute should we phrase the url with JRoute?
*
* @return $this Object to support chaining.
*
* @since 12.2
*/
public function setRedirect($url, $msg = null, $type = 'message', $useJRoute = true)
{
if ($useJRoute)
{
$this->redirect = JRoute::_($url, false);
}
else
{
$this->redirect = $url;
}

if ($msg !== null)
{
$this->message = $msg;
}

$this->messageType = $type;

return $this;
}

/**
* Method to cast all values in a cid array to integer values
* @param array $cid
* @return array $cleanCid
*/
protected function cleanCid($cid)
{
$cleanCid = array();
foreach ((array)$cid AS $id)
{
$cleanCid[] = (int)$id;
}
return $cleanCid;
}

}
49 changes: 49 additions & 0 deletions libraries/cms/controller/cancel.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?php
/**
* @package Joomla.Libraries
* @subpackage Captcha
*
* @copyright Copyright (C) 2005 - 2013 Open Source Matters, Inc. All rights reserved.
* @license GNU General Public License version 2 or later; see LICENSE.txt
*/

defined('JPATH_PLATFORM') or die;

class JCmsControllerCancel extends JCmsControllerBase
{
/**
* (non-PHPdoc)
* @see JController::execute()
*/
public function execute()
{
$config = $this->config;
$url = 'index.php?option='.$config['option'].'&task=display.'.$config['subject'];

$prefix = $this->getPrefix();
$model = $this->getModel($prefix, $config['subject'], $config);
$keyName = $model->getKeyName();

$input = $this->input;
$pk = $input->getInt($keyName, 0);

if ($pk != 0)
{
try
{
$model->checkin($pk);
}
catch (Exception $e)
{
$msg = $e->getMessage();
$this->setRedirect($url, $msg, 'warning');
return false;
}
}

$this->setRedirect($url);
return true;
}


}
Loading