Запрещенные изменения в коде или продолжение истории ремонта одного крана

Данная статья является продолжением ранее опубликованной статьи, которую можно найти здесь.

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

Откладывание рефакторинга на потом ведет к увеличению технического долга и созданию задач (user story) на рефакторинг, которые не имеют business value для product owner-a, а соответственно такие задачи не будут попадать в топ продуктового беклога.

Запрещенные изменения в коде и как их обойти

Удаление класса/интерфейса

Вместо удаления класса либо интерфейса мы отмечаем данную сущность аннотацией @deprecated. Отмечаем также все методы этой сущности как @deprecated для того, чтобы все IDE подсвечивали правильно все deprecated методы.

Причина, по которой сущность стала @deprecated, должна быть указана после аннотации. @see аннотация должна быть использована для рекомендации нового API, которое должно быть использовано вместо устаревшего.

/**
 * @deprecated because new api was introduced
 * @see \New\Api
 */

Мы не можем знать заранее в какой версии продукта будет удален @deprecated код, так как наши планы могут поменяться (мы же Agile), мы можем только знать с какой версии код перестал быть актуальным.
Поэтому чтобы проинформировать сторонних разработчиков о наших планах по изменению публичного кода мы добавляем маркер since вместе с аннотацией @deprecated
Как в примере:

/**
 * @deprecated since 2.1.0
 * @see \Magento\Framework\Model\ResourceModel\Db\AbstractDb::save()
 */
public function save()
{
    // ...
}

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

Удаление public либо protected метода

Вместо удаления метода, который может служить публичным контрактом либо контрактом для наследования, нужно пропаркировать метод как @deprecated. При этом нужно сохранить контракт метода.

Добавление нового метода в класс или интерфейс

Так как Magento не знает как будет использоваться интерфейс как API или как точка расширения (SPI) (подробнее об этом читайте в части 1), то добавление нового метода в контракт также является обратно несовместимым изменением (дальше — BiC), потому что если интерфейс используется как SPI, т.е. сторонние разработчики (дальше — 3PD) предоставляют свою реализацию для него, и подменяют через DI конфигурацию реализацию из коробки. Введя новый метод в контракт сущности — мы принесем проблемы для класса 3PD, у которого нет реализации для нового контракта.

В случае класса — мы всегда можем попасть на коллизию имен, если кто-то наследует этот класс и расширяет его контракт.

В этом случае нужно:

• Создать новый интерфейс с новым методом
• Новый интерфейс может также содержать другие методы, которые содержались в оригинальном классе, если это целесообразно и ведет к увеличению cohesion
• В этом случае соответствующие методы оригинального класса должны быть помечены как @deprecated с добавлением аннотации @see, которая будет указывать на контракт во вновь созданном интерфейсе.
• Старые методы должны проксировать вызовы в методы созданного интерфейса вместо дублирования логики, а также для обеспечения консистентности данных при работе плагинов и ивентов
• Если изменения делаются в рамках PATCH релиза, то новый интерфейс не может быть помечен как @api

Удаление статических функций

Добавление параметров, включая опциональные, в публичные методы

Исходя из того, что 3PD по прежнему могут использовать Inheritance Based API и наследовать Magento классы расширяя их контракт, то добавление параметра в метод может привести к тому, что мы поломаем класс-наследник, который также добавил опциональный параметр к изначальному контракту.

Вместо изменения кода старого метода мы должны ввести новый интерфейс в котором будет указана новая сигнатура метода, которая отвечает нашим измененным бизнес требованиям.
Дальше см. процедуру описанную в пункте «Добавление нового метода в класс или интерфейс».

Добавление параметра в protected метод

Сохраняем метод как есть. Создаем новый метод с новой сигнатурой, и помечаем старый метод как @deprecated. Если возможно создаем новый метод как private.

Изменение типа принимаемых аргументов методом

• Помечаем старый метод как @deprecated
• Вводим новый интерфейс с новым методом, у которого тип аргумента изменен на желаемый
• В аннотации @see старого метода ссылаемся на новый
• Реализация старого метода должна быть изменена таким образом, что полученный старый параметр должен быть преобразован в новый формат, после чего вызов проксируется к вновь созданному интерфейсу

Изменение типа возвращаемого значения методом

Данную задачу можно было бы решить путем возврата типа класса или интерфейса наследника от существующего в контракте метода. Но так как Magento не рекомендует использовать Inheritance Based API для нового кода, то мы не пользуемся такой практикой.

Изменение типа бросаемых исключений

*только в случае если новый тип исключительной ситуации не является подтипом старого контракта.

Изменение контракта конструктора

Для того, чтобы добавить новый параметр в конструктор необходимо сделать параметр опциональным и добавить его последним в список принимаемых аргументов.

В теле конструктора должна быть проверка передана ли новая зависимость, и если новая зависимость не была передана (значение переданного аргумента — null) — извлечь зависимость используя статический метод Magento\Framework\App\ObjectionManager::getInstance().

Пример:

class ExistingClass 
{
    /** 
     * @var \New\Dependency\Interface $newDependency
     */
    private $newDependency;

    public function __construct(
        \Old\Dependency\Intreface $oldDependency,
        $oldRequiredConstructorParameter,
        $oldOptinalConstructorParameter = null,
        \New\Dependency\Interface $newDependency = null
    ) {
        ...
        $this­>newDependency = $newDependency ?: \Magento\Framework\App\ObjectManager::getInstance()
            ->get(\New\Dependency\Interface::class);
        ...
    }

    public function existingFunction() {
        // Existing functionality
            ...
        // Use $this-­>newDependency wherever the new dependency is needed
            ...
    }
}

Всегда ли BC фикс выглядит уродливо (как кран на КДПВ) потому что мы ограничены в стольких вещах?

И как проводить рефакторинг, когда нельзя удалять старые зависимости переданные в конструктор, а можно только добавлять новые. Тем самым мы приближаем Dependency Hell, когда классы принимают огромное колличество внешних зависимостей нарушая при этом SOLID принципы.

Во-первых, нужно категорически запретить подавление ошибок в статических тестах, которые говорят о превышение coupling для вновь созданного кода. Т.е. такие SuppressWarning не должны добавляться после выполнения баг фикса или реализации новой функциональности.

Рефакторинг увеличения Coupling Between Objects и предотвращение Dependency Hell

Если же мы вносим новую валидную зависимость в класс, например, для того чтобы пофиксить баг, и после этого мы переходим порог количества допустимых внешних зависимостей (13).

Мы должны:

• Сохранить обратную совместимость класса, т.е. его Публичный и Протектед интерфейсы. Но должен быть выполнен рефакторинг, который приведет к тому, что части логики класса будут вынесены в отдельные сущности — небольшие специализированные классы.
• Cуществующий класс в этом случае должен выполнять роль фассада, для того чтобы обеспечить корректную работу существующих использований методов, над которыми был произведен рефакторинг.
• Старые public/protected методы должны быть отмечены как @deprecated с указанием @see аннотации на вновь созданные методы в новом классе.
• Все неиспользуемые private свойства класса могут быть удалены
• Все неиспользуемые protected свойства класса должны быть помечены как @deprecated
• Тип в PHP DocBlock неиспользуемой protected переменной свойства класса может быть удален, таким обрразом он не будет посчитан PHPMD как внешняя зависимость.
• Type hinting в конструкторе для неиспользуемой зависимости должен быть удален, таким обрразом он не будет посчитан PHPMD как внешняя зависимость.
• @SuppressWarnings(PHPMD.UnusedFormalParameter) должен быть добавлен для конструктора, чтобы предотвратить срабатывание статической проверки на неиспользуемые аргументы переданные методу

После выполнения действий описанных выше наш код кран засияет новыми красками. И что самое важное — контракт останется таким же как и был до изменений, а соответственно клиенты не заметят этих изменений!