¿Por qué no se permiten tantas funciones PHP en el Estándar de codificación de ECG de Magento?

30

El estándar de codificación de ECG de Magento parece ser (al menos un poco) oficial como estándar para las extensiones de Magento 1:

https://github.com/magento-ecg/coding-standard

Pero no entiendo el razonamiento detrás de todas las reglas, y las reglas de sniffer de código solo con sus mensajes no ayudan mucho. ¿Existe alguna documentación detallada sobre el estándar? Conozco las mejores prácticas comunes y la guía para desarrolladores, pero no puedo encontrar nada específico sobre estos estándares de codificación.

Lo que más me preocupa es la rigurosidad de no usar las funciones de PHP.

Por ejemplo: ¿Por qué está prohibido cada función PHP relacionada con el sistema de archivos ?

Supongo que se supone que debes usar Varien_Io_File, Varien_File_Objectetc. , pero incluso los desarrolladores principales no conocen todas las clases de Varien y a menudo encuentras cosas como en Mage_ImportExport_Model_Import_Adapter_Csv:

    $this->_fileHandler = fopen($this->_source, 'r');

Entonces, el núcleo no es el mejor ejemplo, tan a menudo.

Otras funciones prohibidas cuestionables de mi humilde opinión:

  • mb_parse_str
  • parse_str
  • parse_url
  • base64_decode

    • Sí, se usa en puertas traseras, pero la prohibición evaldebería ser suficiente y hay casos de uso legítimos, como la codificación de datos binarios. Y aparte de json_decode(que no está prohibido) no hay un núcleo de ayuda disponible para esto.

Fuente: https://github.com/magento-ecg/coding-standard/blob/master/Sniffs/Security/ForbiddenFunctionSniff.php

Esencialmente, mi pregunta se reduce a: ¿Dónde está documentado este estándar? ¿Y / o hay una lista de "cosas para usar en lugar de estas funciones PHP nativas"?

Fabian Schmengler
fuente
1
Magento construido sobre Zend Framework. ¿Por qué no usas los estándares Zend?
zhartaunik
ECG realiza más comprobaciones específicas de Magento, como si hay modelos cargados en bucles. No se trata de controles de estilo básicos como sangría y corchetes.
Fabian Schmengler
1
Listado de "consultas SQL sin procesar" como prohibido también parece ingenuo. Por supuesto, no hace SQL sin procesar en la mayoría de las situaciones, pero definitivamente hay momentos en los que no solo es apropiado, sino también necesario (es decir, importaciones / exportaciones complejas)
pspahn
1
@pspahn Veo su punto, pero ¿no debería Zend_Dbser capaz el generador de consultas de generar consultas SQL?
Fabian Schmengler
Claro, pero ¿no puedes crear una selectdeclaración Zend_Dbusando SQL sin procesar como entrada? Supuse que eso es lo que hace github.com/kalenjordan/custom-reports en el backend.
pspahn

Respuestas:

28

Recibí una respuesta no oficial del equipo de ECG sobre eso:

En primer lugar, las funciones marcadas no están necesariamente prohibidas: deben desencadenar una revisión manual del uso para garantizar un uso legítimo. Es una herramienta auxiliar de revisión, no una herramienta de puntuación de código bueno / malo.

En segundo lugar, se supone que es mejor usar envoltorios de Magento (funciones / clases) si existen, ya que podrían ofrecer funcionalidad o protección adicional.

Tercero, en cuanto a llamadas específicas, base64_decode se usa a menudo para código inyectado malicioso, y el resto como parse_str puede ser vulnerable, especialmente el manejo de entradas desconocidas o proporcionadas por el usuario; consulte, por ejemplo, http://php-security.org/2010/05/ 31 / mops-2010-049-php-parse_str-interruption-memory-corrupción-vulnerabilidad /

Nuevamente, esto lo marca para su revisión y no rechaza el código como incorrecto.

Espero eso ayude.

Piotr Kaminski
fuente
Entonces, ¿por qué escriben "la función está prohibida" en lugar de "debe revisar el código para garantizar su uso legítimo"?
Negro
11

El estándar de codificación tiene dos objetivos.

  1. facilita la búsqueda de partes posiblemente problemáticas. Un desarrollador experimentado ya sabe qué partes de un nuevo módulo necesita revisar, y este estándar las marca y las enumera para él. No es para que pueda eliminar estas partes, sino para revisar si son necesarias, problemáticas o ambas.

  2. Apoyar a desarrolladores inexpertos sobre las cosas que debe evitar. Si bien todas las funciones marcadas pueden ser buenas soluciones para problemas específicos, son muy fáciles de usar de forma problemática. Lleva a los desarrolladores a pensar más sobre el problema y, a menudo, a mejores soluciones que no entren en conflicto con el estándar.

Y a veces, incluso los desarrolladores más experimentados siguen ciegamente el estándar y crean las soluciones más crueles, solo para no ofender a un estándar forzado de la comunidad.

un poco de información adicional

Las funciones de archivo a menudo permiten el uso de envolturas de protocolos, significa que una ruta de archivo que comienza con http: // conduce a una solicitud http, que a menudo se usa para "llamar a casa", y de vez en cuando mata a una tienda, porque el servidor no es accesible (Tiempo de espera predeterminado de 30 segundos) y está integrado en un lugar muy central.

todo lo relacionado con sql realizado, nadie cree cuántos agujeros de inyección sql todavía existen por ahí. Un gran ejemplo fue, ya que la Búsqueda en el sitio web de Mysql tenía uno.

hay un núcleo de ayuda para json_decode en alguna parte, pero tiene una implementación muy antigua, solo use json_decode. No tengo idea de por qué debería estar prohibido.

gettext es una parte interesante de php, recuerdo que usa alguna implementación nativa del sistema operativo que podría tener problemas, si lo usa en paralelo con diferentes idiomas (como suele suceder cuando tiene más de un idioma en su tienda. Nunca realmente investigé tanto , pero tampoco debería ser necesario en el contexto de Magento.

Yendo más allá en la lista, parece ser solo una lista con tantas funciones como sea posible. La historia es realmente divertida, parece que eliminaron algunas de las funciones de la lista, después de que se dieron cuenta, la utilizan. :RE

Flyingmana
fuente