Признак доступа к переменным классов, использующих его

Я просто попробовал PHP Traits в первый раз, чтобы сохранить код DRY:

<?php
namespace Acme\DemoBundle\Model;

trait CheckPermissionTrait
{
    protected function checkPermission($object_id)
    {
        $judge = $this->container->get('acme_judge');

        $user  = $this->container->get('security.context')->getToken()->getUser();

        if( !$judge->isPermitted($user, $object_id) ) {
            throw $this->createAccessDeniedException("Brabbel");
        }
    }
}

-

<?php
namespace Acme\DemoBundle\Controller;

use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Symfony\Component\HttpFoundation\Request;
use Acme\DemoBundle\Model\CheckPermissionTrait;

class SomeController extends Controller
{
    use CheckPermissionTrait;

    public function someAction(Request $request, $object_id)
    {
        $this->checkPermission($object_id);

        //do other stuff
    }        
}

Что заставляет меня немного нервничать, так это то, что черта имеет доступ к контейнеру класса, использующего его, что означает, что на самом деле существует требование, чтобы класс, использующий эту черту, должен расширять контроллер (Symfony\Bundle\FrameworkBundle\Controller\Controller), чтобы этот признак работал.

Является ли это нормальным при использовании признаков или плохой практикой?

42 голоса | спросил Marcel Burkhard 18 ThuEurope/Moscow2014-12-18T18:44:13+03:00Europe/Moscow12bEurope/MoscowThu, 18 Dec 2014 18:44:13 +0300 2014, 18:44:13

3 ответа


38

Хотя вы теперь видели свет и знаете, что не используете черты таким образом, я все равно отправлю свой отзыв. Я буду обновлять его по пути, видя, что это то, о чем я могу рассказать некоторое время.

Ваша черта страдает от нескольких (серьезных) проблем:

  • Он представляет собой нарушение договора, и устанавливает плохой контракт.
  • Черты, требующие свойств, похожи на использование global в классе
  • Это не допустимый прецедент для характеристики. На всех.
  • Неправильное пространство имен

Теперь это только первые вещи, о которых я мог думать, каждый из которых достаточно проблематичен, чтобы отбросить черту, как вы по праву сделали.

Позвольте мне объяснить, что означают эти однострочные символы:

Нарушение контракта + принудительное исполнение плохого:

Классы определяют контракт. Это данность. Если вы передадите экземпляр класса X в функцию, вы заключите сделку с этой функцией. Вы по существу говорите «У этого объекта есть это конкретное задание, и вы можете называть эти методы на нем и /или обращаться к этим свойствам» . Когда вы расширяете класс, важно соблюдать контракт. Вы можете добавлять вещи, но никогда не изменяйте обещания, сделанные базовым классом.

Если конструктор базового класса, например, ожидает 1 аргумент массива array (принудительно с помощью подсказки типа), то все его дочерние элементы должны либо использовать один и тот же конструктор, либо, если они выберите его переопределить, определите конструктор, который принимает 1 аргумент array. Они могут потерять подсказку типа, но не могут ее изменить. Они могут добавлять аргументы optional , но не могут принудительно передавать дополнительные аргументы конструктору:

abstract class Base
{
    public function __construct(array $param)
    {}
}
class ValidChild extends Base
{}
class AnotherChild extends Base
{
    public function __construct(array $param, $opt = null)
    {}
}
class IffyButTheoreticallyFine extends Base
{
    public function __construct($param, array $opt = null)
    {}
}

Все эти допустимые дочерние классы: не . Они злые, но, к сожалению, банальные:

class Bastard extends Base
{
    public function __construct()
    {
    }
}
class Ungrateful extends Base
{
    public function __construct(array $param = null)
    {}
}
class EvilTwin extends Base
{
    public function __construct(stdClass $param)
    {
        parent::__construct( (array) $param);
    }
}
class Psychopath extends Base
{
    public function __construct(PDO $db, array $parent = null)
    {
        if (!$parent) {
            $parent = [1, 2, 3];
        }
        parent::__construct($parent);
    }
}

Теперь, как это относится к вашему делу? Простой: черта - инструмент для сокращения дублирования кода. Это самородка часто требуемой функциональности, которую класс /контракт использует /использует для выполнения своей работы. Для того, чтобы класс использовал признак, черта не имеет права определять контракт этого класса. Отношение признака к его пользователю, в некотором роде, такое же, как отношение ребенка к его родительскому: это дочерний элемент, который наследует контракт, а не родительский. Поэтому он должен следить за тем, чтобы любой класс, независимо от его работы, имел , чтобы иметь возможность использовать признак, не изменяя свой собственный контракт и зависимости.

Ваша черта не является допустимым прецедентом: все ее пользователи по определению должны добавить свойство $this->container к своим зависимостям, что, конечно же, будет оказывают влияние на контракт этого класса и контракт его детей.

Новый global при перетаскивании

Я думаю, можно с уверенностью сказать, что общепризнано, что определение класса, содержащее global $someVar, является контрольным признаком плохого кода. Класс ограничен, его методы ограничены, и ни один из них не должен полагаться на глобальную область действия. То же самое относится к признаку trait: потому что он может использоваться в разных классах, а атрибут должен быть агностиком контекста (и области). Если для этого требуется свойство, свойство должно определить его. Если это свойство должно быть определенного типа, единственное, что вы могли бы сделать, это определить свойство и применить для него абстрактный сеттер, который накладывает ограничения типа. Однако это ненадежно, учитывая, что можно использовать несколько признаков, это может привести к конфликтам с пользователем и , что сам класс имеет прямой доступ к этому свойству. Наконец: этот подход возвращает нас к квадрату: черта, подобная этой, больше не является чертой.

Это означает, что для характеристики не может быть зависимостей . Если у черты есть зависимость, она перестает быть чертой: это либо abstract class, либо Interface, но определенно не является признаком. Существует, однако, крошечная полоса серого цвета:

Хотя это не моя идея лучшей практики , можно утверждать, что приемлемо писать черту,можно использовать в сочетании с интерфейсом Interface, например: один интерфейс, обеспечивающий возможность передачи данных, как интерфейс IteratorAggregate. В этом случае такой признак может быть приемлемым возможно .

trait IteratorAggregateTrait
{
    public function getIterator()
    {
        if (!$this instanceof IteratorAggregate) {
            throw new \LogicException(
                sprintf(
                    '%s must implement IteratorAggregate to use %s',
                     get_class($this),
                     __TRAIT__
                )
            );
        }
        return new ArrayIterator($this);
    }
}

Допустимый прецедент

По причинам, которые я объяснял выше, ясно, что ваш пример не лучше всего обрабатывается с помощью признаков. На данном этапе может оказаться полезным выполнить небольшой, 100% действительный пример признака. Предположим, у вас есть некоторые модели, которые представляют собой запись БД, ввод пользователя (данные формы) или что-то еще. Все эти модели содержат информацию о пользователе, включая адрес электронной почты. Модель с сеттерами идеально подходит для обеспечения того, чтобы всякий раз, когда собирались данные, эти данные затем проверялись (т.е. filter_var($email, FILTER_VALIDATE_EMAIL)). Поскольку эти классы представляют разные вещи (запись или форма db), они, скорее всего, наследуют от абстрактного класса. Вместо того, чтобы дублировать установщик электронной почты, можно использовать признак:

trait UserDataTrait
{
    public $email = null;
    public function setEmail($email)
    {
        if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
            throw new \InvalidArgumentException('bad email');
        }
        $this->email = $email;
        return $this;
    }
}

Неверное пространство имен

Немного nit-pick: мне не нравится тот факт, что ваш признак определен в пространстве имен Model, в то время как довольно ясно, что его нельзя использовать нигде, кроме контроллер.


Update

Время обновления. Хотя я твердо (твердо) заявляю ранее (черты не в состоянии наложить контракт), есть, конечно, люди, которые будут утверждать, что могут. Не в последнюю очередь потому, что конструкция PHP trait позволяет объявлять методы abstract:

trait Evil
{
    protected $dependency = null;
    public function someMethod()
    {
        return $this->dependency->getValue();
    }
    abstract public function setDependency(SomeType $dep);
}

Таким образом, эта черта налагает на пользователя частичный контракт: класс, который использует признак должен определить метод setDependency. Так в чем проблема, можно сказать? Проблема проста: были введены черты для решения предполагаемых проблем, которые могут потребовать множественного наследования, поэтому класс может использовать более одного признака, и класс может самостоятельно разрешать конфликты, связанные с именами методов. Итак, давайте предположим, что вышеприведенная черта используется, и у вас есть еще пара признаков, лежащих вокруг, одна из которых выглядит так:

trait Pure
{
    public $dependency = null;
    public function setDependency(Container $container)
    {
        $this->dependency = $container;
        return $this;
    }
}

Теперь представьте, что у вас есть класс, который использует обе эти черты (предположим, что Evil::setDependency не объявлен здесь аббревиатурой):

class FromHell
{
    use Pure, Evil {
        Pure::setDependency insteadof Evil;
    }
}

Поздравляем, контракт, наложенный признаком, сломан, наряду с его функциональностью, просто потому, что класс решил придерживаться контракта, наложенного Pure. Ученый-ракетовед не понимает, что черты, в то время как они могут , теоретически навязывать минимальный контракт, не так сильны в обеспечении того, чтобы контракт соблюдался.

Теперь это был простой пример, но если вы хотите более правдоподобный сценарий: предположите 2 черты, A и B, оба определяют метод logError, A::logError записывается в файл журнала по умолчанию, B является более общим и ожидает, что вы передадите ресурс в качестве аргумента или напишите в stderr по умолчанию.

Поскольку ваш проект довольно велик, вы имеете дело с несколькими уровнями наследования, и в какой-то момент некоторые классы в конечном итоге реализуют оба свойства. Конечно, в зависимости от того, какой класс он имеет, либо A::logError получает более высокий приоритет, либо B:logError. Последний, скорее всего, будет использоваться в коде оператора /crons /backend.

Очень вероятно, что ваш проект, видя, что вы так сильно полагаетесь на черты, содержит код, который выглядит следующим образом:

$traits = class_uses($object);
if (isset($traits['A']))
{
    $object->logError('The error string');
}

Теперь, если этот объект решил переопределитьA::logError с B::logError, вы, вероятно, потратите много времени, пытаясь понять, почему на земле logError doesn ' t напишите сообщения об ошибках в журнал по умолчанию. Короче говоря:

Трейтовые контракты являются слабыми предложениями и могут быть легко переопределены

Вернемся к определению методов abstract в признаке: мне придется протестировать его немного больше, но AFAIK, это просто плохой дизайн. Либо abstract class содержит ваши абстрактные методы (потому что класс применяет сильный контракт), либо вы реализуете метод в классе из get-go. Предполагается, что свойство уменьшает дублирование кода, тогда как абстрактный метод требует, чтобы вы переписывали подпись и реализацию метода всякий раз, когда вы используете этот признак. Если это то, что вы хотите, зачем вообще использовать черту?

Другая проблема, с которой я сталкиваюсь, - это то, что я рассмотрю немного больше, когда найду время, но классы могут быть составлены из 1 или более признаков и не содержат других собственных определений. Это нормально, но что, если мы посмотрим на наши 2 черты выше (Pure и Evil)). Является ли Pure::setDependency допустимой реализацией абстрактного метода Evil::setDependency? Если да, будет ли этот метод признака рассматриваться как реализация абстрактного метода? Если нет, черты, содержащие рефераты, не могут использоваться в классах, которые не содержат собственных определений, и поэтому такие черты являются особыми случаями (поскольку их использование ограничено).

Если Pure::setDependency - действительная реализация абстрактного метода в Evil (Container), являющаяся псевдоним SomeType или его родительский класс), теги и могут использоваться для реализации абстрактных методов trait, затем возникает другая проблема: maintenance HELL . Изменение одной из этих признаков может привести к повреждению вашего кода, в зависимости от того, как и где используются черты. В итоге вам придется протестировать классы all , используя оба , каждый раз, когда вы меняете только один признак. Когда вы посмотрите на это, я бы сказал, что дублирование кода - это меньшее из двух зол.

ответил Elias Van Ootegem 19 FriEurope/Moscow2014-12-19T20:21:07+03:00Europe/Moscow12bEurope/MoscowFri, 19 Dec 2014 20:21:07 +0300 2014, 20:21:07
9

После выполнения некоторых «исследований» я пришел к выводу, что вы не должны использовать Traits так, как я. Мои основные причины этого вывода - плохая тестируемость и удобочитаемость. (Пожалуйста, прочтите первый источник ниже)

Мой код выше (в вопросе) нарушает некоторые принципы дизайна , и я реорганизую мой контроллер для расширения abstract контроллера, который реализует checkPermission Мне нужно, чтобы все контроллеры расширяли ее.

Источники:

Изменить: (2015-07-28)

Прошли месяцы, и я вернусь к этому ответу, и я хочу упомянуть недостаток major в коде. Контроллер выше полагается на весь загружаемый контейнер обслуживания, который называется шаблоном локатора службы .

Зависимости извлекаются путем вызова $this->container->get('name_of_some_service') вместо прямого ввода. Это нарушает закон demeter и приводит к плохой тестируемости и повторному использованию.

Включение зависимостей - это путь туда, и он также поддерживается инфраструктурой symfony.

Появится следующая ссылка: Как определить контроллеры как службы

Используя инъекцию зависимостей, вы получаете более низкую связь с каркасом, зависимости этого класса в большинстве случаев легко видны при взгляде на конструктор.

ответил Marcel Burkhard 19 FriEurope/Moscow2014-12-19T11:42:16+03:00Europe/Moscow12bEurope/MoscowFri, 19 Dec 2014 11:42:16 +0300 2014, 11:42:16
0

Можно перекомпонировать $ this в рамках одного Закрытия, чтобы вы могли получить доступ к контейнеру любого переданного объекта

function isJudgePermitted($object, int $object_id) : bool {
  return (function (int $object_id) : bool {
    $judge = $this->container->get('acme_judge');
    $user = $this->container->get('security.context')->getToken()->getUser();
    return $judge->isPermitted($user, $object_id);
  })->call($object, $object_id);
}

// Inside SomeController.

public function someAction(Request $request, $object_id) {
  $judge_permitted = isJudgePermitted($this, $object_id);
  $judge_permitted ?: throw $this->createAccessDeniedException("Brabbel");
  //do other stuff
}   
ответил arcanine 16 PM00000010000004631 2017, 13:04:46

Похожие вопросы

Популярные теги

security × 330linux × 316macos × 2827 × 268performance × 244command-line × 241sql-server × 235joomla-3.x × 222java × 189c++ × 186windows × 180cisco × 168bash × 158c# × 142gmail × 139arduino-uno × 139javascript × 134ssh × 133seo × 132mysql × 132