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

Added a new collector for Hooks #8327

Merged
merged 2 commits into from Sep 18, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 44 additions & 0 deletions classes/Hook.php
Expand Up @@ -25,6 +25,7 @@
*/

use PrestaShop\PrestaShop\Core\Module\WidgetInterface;
use PrestaShop\PrestaShop\Adapter\SymfonyContainer;

class HookCore extends ObjectModel
{
Expand Down Expand Up @@ -705,6 +706,14 @@ public static function exec(
return;
}

$hookRegistry = self::getHookRegistry();
$isRegistryEnabled = !is_null($hookRegistry);

if ($isRegistryEnabled) {
$backtrace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 1);
$hookRegistry->selectHook($hook_name, $hook_args, $backtrace[0]['file'], $backtrace[0]['line']);
}

// $chain & $array_return are incompatible so if chained is set to true, we disable the array_return option
if (true === $chain) {
$array_return = false;
Expand All @@ -723,6 +732,9 @@ public static function exec(
// If no modules associated to hook_name or recompatible hook name, we stop the function

if (!$module_list = Hook::getHookModuleExecList($hook_name)) {
if ($isRegistryEnabled) {
$hookRegistry->collect();
}
if ($array_return) {
return array();
} else {
Expand All @@ -732,6 +744,9 @@ public static function exec(

// Check if hook exists
if (!$id_hook = Hook::getIdByName($hook_name)) {
if ($isRegistryEnabled) {
$hookRegistry->collect();
}
if ($array_return) {
return array();
} else {
Expand Down Expand Up @@ -827,6 +842,10 @@ public static function exec(
continue;
}

if ($isRegistryEnabled) {
$hookRegistry->hookedByModule($moduleInstance);
}

if (Hook::isHookCallableOn($moduleInstance, $hook_name)) {
$hook_args['altern'] = ++$altern;

Expand All @@ -849,6 +868,9 @@ public static function exec(
$output .= $display;
}
}
if ($isRegistryEnabled) {
$hookRegistry->hookedByCallback($moduleInstance, $hook_args);
}
} elseif (Hook::isDisplayHookName($hook_name)) {
if ($moduleInstance instanceof WidgetInterface) {

Expand All @@ -868,6 +890,10 @@ public static function exec(
}
}
}

if ($isRegistryEnabled) {
$hookRegistry->hookedByWidget($moduleInstance, $hook_args);
}
}
}

Expand All @@ -885,6 +911,11 @@ public static function exec(
}
}

if ($isRegistryEnabled) {
$hookRegistry->hookWasCalled();
$hookRegistry->collect();
}

return $output;
}

Expand All @@ -897,4 +928,17 @@ public static function coreRenderWidget($module, $hook_name, $params)
{
return $module->renderWidget($hook_name, $params);
}

/**
* @return null|\PrestaShopBundle\DataCollector\HookRegistry
*/
private static function getHookRegistry()
{
$sfContainer = SymfonyContainer::getInstance();
if (!is_null($sfContainer) && "dev" === $sfContainer->getParameter('kernel.environment')) {
return $sfContainer->get('prestashop.hooks_registry');
}

return null;
}
}
3 changes: 2 additions & 1 deletion classes/module/Module.php
Expand Up @@ -28,8 +28,9 @@
use PrestaShop\PrestaShop\Adapter\Module\ModuleDataProvider;
use PrestaShop\PrestaShop\Core\Module\WidgetInterface;
use PrestaShop\PrestaShop\Adapter\ServiceLocator;
use PrestaShop\PrestaShop\Core\Module\ModuleInterface;

abstract class ModuleCore
abstract class ModuleCore implements ModuleInterface
Copy link
Member

Choose a reason for hiding this comment

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

Why do we add this interface? If the legacy class is supposed to disappear, I do not understand this change.

{
/** @var int Module ID */
public $id = null;
Expand Down
35 changes: 35 additions & 0 deletions src/Core/Module/ModuleInterface.php
@@ -0,0 +1,35 @@
<?php
/**
* 2007-2017 PrestaShop
*
* NOTICE OF LICENSE
*
* This source file is subject to the Open Software License (OSL 3.0)
* that is bundled with this package in the file LICENSE.txt.
* It is also available through the world-wide-web at this URL:
* https://opensource.org/licenses/OSL-3.0
* If you did not receive a copy of the license and are unable to
* obtain it through the world-wide-web, please send an email
* to license@prestashop.com so we can send you a copy immediately.
*
* DISCLAIMER
*
* Do not edit or add to this file if you wish to upgrade PrestaShop to newer
* versions in the future. If you wish to customize PrestaShop for your
* needs please refer to http://www.prestashop.com for more information.
*
* @author PrestaShop SA <contact@prestashop.com>
* @copyright 2007-2017 PrestaShop SA
* @license https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
* International Registered Trademark & Property of PrestaShop SA
*/

namespace PrestaShop\PrestaShop\Core\Module;

/**
* Define what should be a module.
* Note:We don't typeHint on old Module class to not create hard dependency with Legacy.
*/
interface ModuleInterface
{
}
120 changes: 120 additions & 0 deletions src/PrestaShopBundle/DataCollector/HookDataCollector.php
@@ -0,0 +1,120 @@
<?php
/**
* 2007-2017 PrestaShop
*
* NOTICE OF LICENSE
*
* This source file is subject to the Open Software License (OSL 3.0)
* that is bundled with this package in the file LICENSE.txt.
* It is also available through the world-wide-web at this URL:
* https://opensource.org/licenses/OSL-3.0
* If you did not receive a copy of the license and are unable to
* obtain it through the world-wide-web, please send an email
* to license@prestashop.com so we can send you a copy immediately.
*
* DISCLAIMER
*
* Do not edit or add to this file if you wish to upgrade PrestaShop to newer
* versions in the future. If you wish to customize PrestaShop for your
* needs please refer to http://www.prestashop.com for more information.
*
* @author PrestaShop SA <contact@prestashop.com>
* @copyright 2007-2017 PrestaShop SA
* @license https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
* International Registered Trademark & Property of PrestaShop SA
*/

namespace PrestaShopBundle\DataCollector;

use Symfony\Component\HttpKernel\DataCollector\DataCollector;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;

/**
* Collect all information about Legacy hooks and make it available
* in the Symfony Web profiler.
*/
final class HookDataCollector extends DataCollector
{
/**
* @var HookRegistry
*/
private $registry;

public function __construct(HookRegistry $registry)
{
$this->registry = $registry;
}

/**
* @{inheritdoc}
*/
public function collect(Request $request, Response $response, \Exception $exception = null)
{
$hooks = $this->registry->getHooks();
$calledHooks = $this->registry->getCalledHooks();
$notCalledHooks = $this->registry->getNotCalledHooks();
$this->data = array(
'hooks' => $this->stringifyHookArguments($hooks),
'calledHooks' => $this->stringifyHookArguments($calledHooks),
'notCalledHooks' => $this->stringifyHookArguments($notCalledHooks),
);
}

/**
* Return the list of every dispatched legacy hooks during one request.
* @return array
*/
public function getHooks()
{
return $this->data['hooks'];
}

/**
* Return the list of every called legacy hooks during one request.
* @return array
*/
public function getCalledHooks()
{
return $this->data['calledHooks'];
}

/**
* Return the list of every uncalled legacy hooks during oHookne request.
* @return array
*/
public function getNotCalledHooks()
{
return $this->data['notCalledHooks'];
}

/**
* @{inheritdoc}
*/
public function getName()
{
return 'ps.hooks_collector';
Copy link
Member

Choose a reason for hiding this comment

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

I think this value should be stored in a constant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why? this value is only reused on service declaration.

Copy link
Member

Choose a reason for hiding this comment

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

The way I see it, constants are not (only) about reuse, they are also about not having important pieces of information stored as literals buried in the code.

One of the advantages of constants is that they are declared at the top of the class in an organized manner, which in my opinion improves clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, this don't apply here, this information is totaly useless for the developer and will increase what we call "visual debt" :)

}

/**
* @param array $hooksList
* @return array a better representation of arguments for HTML rendering.
*/
private function stringifyHookArguments(array &$hooksList)
{
foreach ($hooksList as &$hookList) {
foreach ($hookList as &$hook) {
$hook['args'] = $this->varToString($hook['args']);
foreach ($hook['modules'] as &$modulesByType) {
foreach ($modulesByType as &$module) {
if (array_key_exists('args', $module)) {
$module['args'] = $this->varToString($module['args']);
}
}
}
}
}

return $hooksList;
}
}