bagisto-code-review

代码审查

审查 Bagisto 变更时需要关注的事项，按重要程度排序，让最重要的问题先浮现。Pint 和测试套件已经能判断机械性问题——把审查精力放在它们看不到的地方。

工具已经覆盖的内容

不要花审查精力检查以下内容；运行相应工具即可。

检查工具	覆盖内容
vendor/bin/pint --test	格式、空格、导入顺序、尾随逗号
vendor/bin/pest	测试套件断言的行为
php artisan bagisto:translations:check	23 种语言环境中缺失的键
Playwright	浏览器层

如果一条审查意见本可以由退出码给出，解决方法就是运行工具，而不是写下这条意见。

阻塞合并的问题

以下问题未解决时，不应合并变更。

正确性

• 在循环中执行查询，或因缺少预加载而产生 N+1 查询。在种子数据上看不出成本，到了真实商品目录中就会显现。
• 对持续增长的表执行不受限制的 ->get()，例如商品、订单、客户表。请分页或分块处理。
• 仓储方法访问卖家或客户拥有的行，却没有按所有者限定范围。
• 通过插入请求值构建原始 SQL 片段（whereRaw、selectRaw、DB::raw、orderByRaw）。

安全性

安全性由 bagisto-coding-standards 技能负责——如果差异涉及授权、渲染输出、输入、上传、原始 SQL、密钥或付款，请加载该技能，并按其检查清单逐项核查，而不是依赖记忆。以下两项涵盖了大多数实际问题：

• 店面查询使用请求中的 id 进行筛选，却没有限定为已通过身份验证的客户。
• DataGrid 闭包将值插入 HTML 属性，却没有使用 e()——标签会为你剥除，但引号不会。

架构

• 在仓储之外使用 DB:: 或模型查询。唯一获准的例外是 DataGrid 的 prepareQueryBuilder()。
• 新建模型却没有对应的 Contract、Model、Proxy 和 Repository。
• 在 bootstrap/providers.php 中注册了程序包，却未在 config/concord.php 中注册，或反之。
• 为扩展功能而编辑核心文件。Marketplace 和 B2B Suite 都绝不允许修改核心文件——这类变更应放在绑定的子类、view_render_event 监听器或 Concord 模型替换中。
• 面向用户的字符串没有通过 trans() 处理。

值得提出，但不阻塞合并的问题

• 第三次出现时才算重复。 两次只是巧合；三次就该抽成辅助方法。
• 方法体需要注释才能看懂。 代码库禁止在方法体内添加注释，因此这说明应提取一个命名明确的方法，而不是添加文字说明。
• 变更涉及的文件违反了文档块或成员顺序规范。 触及文件中原本就存在的违规，由作者负责修复。
• 测试断言数量或列表位置。 随着共享数据库增长，两者都会变化；应改为断言指定记录。
• 单条记录路径会触发事件，但批量操作路径不会触发，或反之。

如何审查

1. 先阅读测试。 测试说明作者认为变更实现了什么。若修复的 bug 没有对应测试，这是首先要提出的问题。
2. 思考什么会让它出错。 对每个断言，寻找能推翻它的输入——空集合、第二种语言环境、访客、非默认渠道。
3. 检查反向情况。 撤销修复后仍能通过的回归测试毫无保护作用。若修复不易察觉，请作者展示测试失败的情况。
4. 端到端地跟踪一条路径——请求、表单请求、控制器、仓储、模型、视图——而不是逐块阅读差异。大多数实际缺陷都出现在两个各自看似正常的文件之间的衔接处。
5. 确认各项检查都已运行，并了解哪些被跳过。

编写问题描述

先说明缺陷，再说明触发缺陷的输入，最后说明修复方法。没有具体故障的问题只是偏好，应标记为偏好。

CategoryRepository::saveMediaAltText() 会写入 translateOrNew($locale)，其中 $locale 可能是字面值 all—— 分类创建表单会提交 locale=all。在 MySQL 上，这会静默地创建一条 name 和 slug 为空的记录；在 PostgreSQL 上，则会违反 NOT NULL 约束。写入前应将此哨兵值扩展为所有语言区域。

将已验证的内容与推测分开。“这是 N+1”与“这可能是 N+1，我没有检查关联关系”是不同的说法；将两者混为一谈，比如实说明情况更耽误作者时间。

必需的子技能： 在认定任何更改已完成之前，使用 bagisto-change-verification。