Skip to content
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
2c7728e
Introduce router service
laoneo May 10, 2018
e94cc78
CS
laoneo May 10, 2018
2f64e1d
Merge branch '4.0-dev' into j4/router/service
laoneo May 10, 2018
092c026
Merge branch '4.0-dev' into j4/router/service
wilsonge May 10, 2018
5ebf3b3
Merge remote-tracking branch 'remotes/upstream/4.0-dev' into j4/route…
laoneo May 31, 2018
665b6cf
CS
laoneo May 31, 2018
afd9c73
Changed types
laoneo May 31, 2018
156a388
Only support site application
laoneo May 31, 2018
b44e8ad
Types
laoneo May 31, 2018
329a20c
Adapt legacy component
laoneo May 31, 2018
0cdfc17
Merge branch '4.0-dev' into j4/router/service
laoneo May 31, 2018
99550a7
Merge branch '4.0-dev' into j4/router/service
laoneo Jun 15, 2018
dbf7300
Merge remote-tracking branch 'remotes/upstream/4.0-dev' into j4/route…
laoneo Jul 17, 2018
73d9098
Merge remote-tracking branch 'origin/j4/router/service' into j4/route…
laoneo Jul 17, 2018
625f730
Merge branch '4.0-dev' into j4/router/service
laoneo Jul 17, 2018
df49591
fix conflicts
laoneo Aug 6, 2018
6fd4f9f
Merge remote-tracking branch 'origin/j4/router/service' into j4/route…
laoneo Aug 6, 2018
76d5c06
Merge branch '4.0-dev' into j4/router/service
laoneo Aug 7, 2018
ac82176
Merge conflicts
laoneo Aug 15, 2018
a9d5e6e
Merge branch '4.0-dev' into j4/router/service
laoneo Aug 20, 2018
6d50d75
Merge branch '4.0-dev' into j4/router/service
laoneo Aug 21, 2018
868895c
Merge branch '4.0-dev' into j4/router/service
laoneo Aug 22, 2018
eed26f9
Merge branch '4.0-dev' into j4/router/service
laoneo Aug 23, 2018
36769de
Merge branch '4.0-dev' into j4/router/service
laoneo Aug 24, 2018
afae590
fix conflicts
laoneo Sep 3, 2018
531b693
Merge remote-tracking branch 'origin/j4/router/service' into j4/route…
laoneo Sep 3, 2018
9722661
Merge branch '4.0-dev' into j4/router/service
laoneo Sep 13, 2018
9f0298c
Merge branch '4.0-dev' into j4/router/service
laoneo Sep 19, 2018
2499795
fix conflicts
laoneo Oct 1, 2018
085953e
Merge remote-tracking branch 'origin/j4/router/service' into j4/route…
laoneo Oct 1, 2018
80bd410
Add namespaced factory
laoneo Oct 1, 2018
dd75148
Merge remote-tracking branch 'remotes/upstream/4.0-dev' into j4/route…
laoneo Oct 1, 2018
1cfca61
revert
laoneo Oct 1, 2018
111a333
default to empty section
laoneo Oct 1, 2018
404991f
organize
laoneo Oct 1, 2018
df353dc
contact router
laoneo Oct 1, 2018
4085469
finder
laoneo Oct 1, 2018
d8f196c
newsfeeds
laoneo Oct 1, 2018
c1eaca8
users
laoneo Oct 1, 2018
5b25d40
finder
laoneo Oct 1, 2018
c956108
Save
laoneo Oct 1, 2018
f6ab98c
stabilize
laoneo Oct 1, 2018
550de78
Merge remote-tracking branch 'remotes/upstream/4.0-dev' into j4/route…
laoneo Oct 1, 2018
8ea0ada
fixes
laoneo Oct 1, 2018
5621d7c
Merge remote-tracking branch 'remotes/upstream/4.0-dev' into j4/route…
laoneo Oct 2, 2018
bc69b61
Merge remote-tracking branch 'origin/j4/router/service' into j4/route…
laoneo Oct 2, 2018
9dcd4c2
alpha order imports
laoneo Oct 2, 2018
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
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,25 @@
use Psr\Container\ContainerInterface;
use Joomla\CMS\Language\Text;
use Joomla\CMS\Factory;
use Joomla\CMS\Component\Router\RouterServiceInterface;
use Joomla\CMS\Component\Router\RouterServiceTrait;

/**
* Component class for com_content
*
* @since __DEPLOY_VERSION__
*/
class ContentComponent extends MVCComponent implements
BootableExtensionInterface, MVCFactoryServiceInterface, CategoriesServiceInterface, FieldsServiceInterface,
AssociationServiceInterface, WorkflowServiceInterface
BootableExtensionInterface,
CategoriesServiceInterface,
FieldsServiceInterface,
AssociationServiceInterface,
RouterServiceInterface,
WorkflowServiceInterface
{
use CategoriesServiceTrait;
use AssociationServiceTrait;
use RouterServiceTrait;
use HTMLRegistryAwareTrait;
use WorkflowServiceTrait;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<?php
/**
* Joomla! Content Management System
*
* @copyright Copyright (C) 2005 - 2017 Open Source Matters, Inc. All rights reserved.
* @license GNU General Public License version 2 or later; see LICENSE.txt
*/

namespace Joomla\Component\Content\Administrator\Service\Provider;

defined('JPATH_PLATFORM') or die;

use Joomla\DI\Container;
use Joomla\DI\ServiceProviderInterface;
use Joomla\CMS\Component\Router\RouterFactoryInterface;
use Joomla\CMS\Categories\Categories;
use Joomla\Database\DatabaseInterface;

/**
* Service provider for the service dispatcher factory.
*
* @since __DEPLOY_VERSION__
*/
class RouterFactory implements ServiceProviderInterface
{
/**
* Registers the service provider with a DI container.
*
* @param Container $container The DI container.
*
* @return void
*
* @since __DEPLOY_VERSION__
*/
public function register(Container $container)
{
$container->set(
RouterFactoryInterface::class,
function (Container $container)
{
return new \Joomla\Component\Content\Site\Router\RouterFactory(
$container->get(Categories::class)[''],
$container->get(DatabaseInterface::class)
);
}
);
}
}
4 changes: 4 additions & 0 deletions administrator/components/com_content/services/provider.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
use Joomla\Component\Content\Site\Service\Category;
use Joomla\DI\Container;
use Joomla\DI\ServiceProviderInterface;
use Joomla\CMS\Component\Router\RouterFactoryInterface;
use Joomla\Component\Content\Administrator\Service\Provider\RouterFactory;

/**
* The content service provider.
Expand All @@ -46,6 +48,7 @@ public function register(Container $container)

$container->registerServiceProvider(new MVCFactoryFactory('\\Joomla\\Component\\Content'));
$container->registerServiceProvider(new DispatcherFactory('\\Joomla\\Component\\Content'));
$container->registerServiceProvider(new RouterFactory);

$container->set(
ComponentInterface::class,
Expand All @@ -57,6 +60,7 @@ function (Container $container)
$component->setMvcFactoryFactory($container->get(MVCFactoryFactoryInterface::class));
$component->setCategories($container->get(Categories::class));
$component->setAssociationExtension($container->get(AssociationExtensionInterface::class));
$component->setRouterFactory($container->get(RouterFactoryInterface::class));

return $component;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,20 @@
* @license GNU General Public License version 2 or later; see LICENSE.txt
*/

namespace Joomla\Component\Content\Site\Router;

defined('_JEXEC') or die;

use Joomla\CMS\Application\CMSApplication;
use Joomla\CMS\Application\SiteApplication;
use Joomla\CMS\Categories\Categories;
use Joomla\CMS\Component\ComponentHelper;
use Joomla\CMS\Component\Router\RouterView;
use Joomla\CMS\Component\Router\RouterViewConfiguration;
use Joomla\CMS\Component\Router\Rules\MenuRules;
use Joomla\CMS\Component\Router\Rules\NomenuRules;
use Joomla\CMS\Component\Router\Rules\StandardRules;
use Joomla\CMS\Menu\AbstractMenu;
use Joomla\CMS\Categories\Categories;
use Joomla\CMS\Factory;
use Joomla\Database\DatabaseInterface;

/**
* Routing class of com_content
Expand All @@ -29,14 +31,37 @@ class ContentRouter extends RouterView
{
protected $noIDs = false;

/**
* The category
*
* @var Categories
*
* @since __DEPLOY_VERSION__
*/
private $category;

/**
* The db
*
* @var DatabaseInterface
*
* @since __DEPLOY_VERSION__
*/
private $db;

/**
* Content Component router constructor
*
* @param CMSApplication $app The application object
* @param AbstractMenu $menu The menu object to work with
* @param SiteApplication $app The application object
* @param AbstractMenu $menu The menu object to work with
* @param Categories $category The category object
* @param DatabaseInterface $db The database object
*/
public function __construct($app = null, $menu = null)
public function __construct(SiteApplication $app, AbstractMenu $menu, Categories $category, DatabaseInterface $db)
Copy link
Member

Choose a reason for hiding this comment

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

I strongly disagree with this here. 90% of the components out there don't have categories and also don't need DB access. Even if you could hand in dummy objects, this is wrong. Also, you are not handing in one category object, but an object to get any category from.

I would rather have something like a setDb() and setCategories() to fill this, than handing this over in the constructor. Or add this as an additional interface or something like that.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, now I see what you are doing here and my previous comment about category and db as parameters was wrong. But please rename $category to something else. It is not a single category that you get. On a side note: We should improve that part and name JCategories to something better...

Copy link
Member Author

Choose a reason for hiding this comment

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

I did something into that direction with a new CategoryInterface here https://github.com/joomla/joomla-cms/pull/20693/files#diff-d1bc8815185a7ce4f6b7cff706b71f48R18. Any comments are welcome.

{
$this->category = $category;
$this->db = $db;

$params = ComponentHelper::getParams('com_content');
$this->noIDs = (bool) $params->get('sef_ids');
$categories = new RouterViewConfiguration('categories');
Expand Down Expand Up @@ -71,7 +96,8 @@ public function __construct($app = null, $menu = null)
*/
public function getCategorySegment($id, $query)
{
$category = Categories::getInstance($this->getName())->get($id);
$this->category->setOptions(array('access' => true));
$category = $this->category->get($id);

if ($category)
{
Expand Down Expand Up @@ -117,14 +143,13 @@ public function getArticleSegment($id, $query)
{
if (!strpos($id, ':'))
{
$db = Factory::getDbo();
$dbquery = $db->getQuery(true);
$dbquery = $this->db->getQuery(true);
$dbquery->select($dbquery->qn('alias'))
->from($dbquery->qn('#__content'))
->where('id = ' . $dbquery->q($id));
$db->setQuery($dbquery);
$this->db->setQuery($dbquery);

$id .= ':' . $db->loadResult();
$id .= ':' . $this->db->loadResult();
}

if ($this->noIDs)
Expand Down Expand Up @@ -164,7 +189,9 @@ public function getCategoryId($segment, $query)
{
if (isset($query['id']))
{
$category = Categories::getInstance($this->getName(), array('access' => false))->get($query['id']);
$this->category->setOptions(array('access' => false));

$category = $this->category->get($query['id']);

if ($category)
{
Expand Down Expand Up @@ -216,15 +243,14 @@ public function getArticleId($segment, $query)
{
if ($this->noIDs)
{
$db = Factory::getDbo();
$dbquery = $db->getQuery(true);
$dbquery = $this->db->getQuery(true);
$dbquery->select($dbquery->qn('id'))
->from($dbquery->qn('#__content'))
->where('alias = ' . $dbquery->q($segment))
->where('catid = ' . $dbquery->q($query['id']));
$db->setQuery($dbquery);
$this->db->setQuery($dbquery);

return (int) $db->loadResult();
return (int) $this->db->loadResult();
}

return (int) $segment;
Expand Down
79 changes: 79 additions & 0 deletions components/com_content/Router/RouterFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
<?php
/**
* Joomla! Content Management System
*
* @copyright Copyright (C) 2005 - 2017 Open Source Matters, Inc. All rights reserved.
* @license GNU General Public License version 2 or later; see LICENSE
*/

namespace Joomla\Component\Content\Site\Router;

defined('_JEXEC') or die;

use Joomla\CMS\Application\CMSApplicationInterface;
use Joomla\CMS\Application\SiteApplication;
use Joomla\CMS\Component\Router\RouterFactoryInterface;
use Joomla\CMS\Component\Router\RouterInterface;
use Joomla\CMS\Menu\AbstractMenu;
use Joomla\Component\Content\Site\Service\Category;
use Joomla\Database\DatabaseInterface;

/**
* Content router factory.
*
* @since __DEPLOY_VERSION__
*/
class RouterFactory implements RouterFactoryInterface
{
/**
* The category
*
* @var Category
*
* @since __DEPLOY_VERSION__
*/
private $category;

/**
* The db
*
* @var DatabaseInterface
*
* @since __DEPLOY_VERSION__
*/
private $db;

/**
* Content Component router factory constructor
*
* @param Category $category The category object
* @param DatabaseInterface $db The database object
*
* @since __DEPLOY_VERSION__
*/
public function __construct(Category $category, DatabaseInterface $db)
{
$this->category = $category;
$this->db = $db;
}

/**
* Creates a router.
*
* @param CMSApplicationInterface $application The application
* @param AbstractMenu $menu The menu object to work with
*
* @return RouterInterface
*
* @since __DEPLOY_VERSION__
*/
public function createRouter(CMSApplicationInterface $application, AbstractMenu $menu): RouterInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have a SiteApplication here (we are aborting if we don't) - why can't we use it's getMenu method? Is there ever a time we need any other instance?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea behind it was to not tight us to the a specific application so I created the interface definition generic. Makes it more flexible. The ContentRouter factory does only provide a router for a SiteApplication, that's why I'v added a check.

Copy link
Contributor

Choose a reason for hiding this comment

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

The ContentRouter factory does only provide a router for a SiteApplication

Are there going to be any Routers in Joomla that provide a router for more than SiteApplication?

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, what is the actual need for the app in component routers? It's not used in the RouterBase or RouterView classes for anything other than fetching the menu if one isn't injected.

Copy link
Contributor

Choose a reason for hiding this comment

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

The component router rules have hard references to the public $menu property (I really wish we would've done this as a getter, but ¯\_(ツ)_/¯. The component router rules, and most of the core component routers themselves have no use of the application. I think this argument should be dropped personally, unfortunately to do it right means getting tricky with the RouterBase constructor to deprecate the app argument/property as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Dropping the app would be a BC break. Don't know if we really want to do that. But you are right, it basically acts only as fallback on a quick glance. But can we discus that in it's own pr, otherwise it will be a never ending story.

Copy link
Contributor

@mbabker mbabker Aug 31, 2018

Choose a reason for hiding this comment

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

You can do it without a hard B/C break, it's just complicated at the code level.

public function __construct(/* AbstractMenu */ $menu)
{
    $args = func_get_args();

    if (count($args) > 1) {
        @trigger_error(sprintf('Passing an application object to %s is deprecated and will be removed in 5.0, a %s object will be required at that point.', get_class($this), AbstractMenu::class), E_USER_DEPRECATED);

        $app = $args[0];
        $menu = $args[1] ?: $app->getMenu();
    } elseif ($menu) {
        if (!($menu instanceof AbstractMenu)) {
            if (!($menu instanceof CMSApplicationInterface)) {
                throw new \InvalidArgumentException(sprintf('The first argument for the constructor of %s must be a %s or %s instance.', __CLASS__, CMSApplicationInterface::class, AbstractMenu::class));
            }

            @trigger_error(sprintf('Passing an application object to %s is deprecated and will be removed in 5.0, a %s object will be required at that point.', get_class($this), AbstractMenu::class), E_USER_DEPRECATED);

            $app = $menu;
            $menu = $app->getMenu();
        } else {
            $app = Factory::getApplication();
        }
    } else {
        @trigger_error(sprintf('Passing no arguments to the constructor of %s is deprecated. As of 5.0, a %s object will be required.', get_class($this), AbstractMenu::class), E_USER_DEPRECATED);

        $app = Factory::getApplication();
        $menu = $app->getMenu();
    }

    $this->app = $app;
    $this->menu = $menu;
}

{
if (!$application instanceof SiteApplication)
{
throw new \RuntimeException('No router available for this application.');
}

return new ContentRouter($application, $menu, $this->category, $this->db);
}
}
34 changes: 34 additions & 0 deletions libraries/src/Component/Router/RouterFactoryInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php
/**
* Joomla! Content Management System
*
* @copyright Copyright (C) 2005 - 2017 Open Source Matters, Inc. All rights reserved.
* @license GNU General Public License version 2 or later; see LICENSE.txt
*/

namespace Joomla\CMS\Component\Router;

defined('_JEXEC') or die;

use Joomla\CMS\Application\CMSApplicationInterface;
use Joomla\CMS\Menu\AbstractMenu;

/**
* Router factory interface
*
* @since __DEPLOY_VERSION__
*/
interface RouterFactoryInterface
{
/**
* Creates a router.
*
* @param CMSApplicationInterface $application The application
* @param AbstractMenu $menu The menu object to work with
*
* @return RouterInterface
*
* @since __DEPLOY_VERSION__
*/
public function createRouter(CMSApplicationInterface $application, AbstractMenu $menu): RouterInterface;
}
34 changes: 34 additions & 0 deletions libraries/src/Component/Router/RouterServiceInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php
/**
* Joomla! Content Management System
*
* @copyright Copyright (C) 2005 - 2017 Open Source Matters, Inc. All rights reserved.
* @license GNU General Public License version 2 or later; see LICENSE.txt
*/

namespace Joomla\CMS\Component\Router;

defined('JPATH_PLATFORM') or die;

use Joomla\CMS\Application\CMSApplicationInterface;
use Joomla\CMS\Menu\AbstractMenu;

/**
* The component router service.
*
* @since __DEPLOY_VERSION__
*/
interface RouterServiceInterface
{
/**
* Returns the router.
*
* @param CMSApplicationInterface $application The application object
* @param AbstractMenu $menu The menu object to work with
*
* @return RouterInterface
*
* @since __DEPLOY_VERSION__
*/
public function createRouter(CMSApplicationInterface $application, AbstractMenu $menu): RouterInterface;
}
Loading