前几天朋友甩给我一个 ThinkPHP 5.1 的企业官网项目，让我帮忙看看有没有问题。我花了大半天把代码从头到尾翻了一遍，不看不知道，一看真是处处惊喜。
记录一下发现的问题和修复过程，给同样要接手老项目的兄弟们一个参考。
项目背景
这是一个典型的外贸企业官网，ThinkPHP 5.1 + MySQL，前台展示产品、新闻、解决方案，后台管理内容。代码量不大，48 个 PHP 文件，20 多个后台控制器。
看起来是小团队快速开发的，功能都有，但代码质量参差不齐。
最离谱的 Bug：删除按钮删错表
后台有个服务团队管理模块，Team 控制器的删除方法长这样：
$this->delRecord('temp', $id, '删除服务团队：')注意看，表名写的是 temp，不是 team。也就是说用户在后台点删除，根本删不掉任何东西，因为 temp 表压根不存在。
这种错误估计是复制粘贴的时候忘改了，但上线这么久居然没人发现，说明要么没人用这个功能，要么用了也没反馈。
软删除和真删除混用
项目大部分模块用的是软删除（设置 is_del=1），但有几个地方画风突变：
- Banner 轮播图 — 直接
->delete()，数据删了就没了 - 公司历史 — 也是真删除
- 用户和角色 — 还是真删除
同一个项目里，有的模块用软删除有的用真删除，纯粹看开发者当时心情。修复很简单，统一改成软删除就行。
日志写错
公司历史的编辑方法里，操作日志写的是：
insert_system_log('添加公司历史：' . $id);明明是编辑操作，日志却记成了「添加」。还有 Solution 控制器的状态变更日志，$log_msg 是空字符串，等于啥都没记。
系统日志是出了问题排查的依据，写错了跟没写一样。
安全隐患：表名由前端传
Category 控制器有个 changeStatus 方法，表名参数直接从前端传过来：
public function changeStatus($table, $field, $type, $id)意思是前端可以传任意表名进来修改任意表的状态字段。虽然后台有登录验证，但这种写法还是很危险。
修复方式是加个白名单：
$allowed = ['product_type', 'company_picture_type', 'news_type', 'oem_type'];
if (!in_array($table, $allowed)) {
return json(['code' => 'err', 'msg' => '非法操作']);
}前台 getTree() 的 static 变量坑
前台 Common.php 里有个 getTree() 方法，用了 static $arr 来存结果。问题是同一个请求里如果调用两次 getTree()，第二次会把第一次的结果也带上，因为 static 变量不会自动重置。
首页刚好调用了两次这个方法（一次生成导航，一次生成分类），所以分类列表里会莫名多出导航的数据。
下载功能没有权限校验
前台的文档下载方法，拿到 id 就直接查数据库下载：
$info = db('doc')->where('id', $id)->find();
return download('.' . $info['file'], $info['name']);没有检查文档是否已发布、是否已删除。随便改个 id 就能下载任意文档，包括未发布的内部文件。
编辑的时候覆盖了创建时间
Company 控制器的 editPosition 方法里：
$data['create_time'] = time();
$data['update_time'] = time();编辑操作不应该改创建时间，这样每次编辑都会把创建时间刷新成当前时间，数据就不准了。
Products Model 里的 $this->db()
在 ThinkPHP 5.1 的 Model 里，$this->db() 和全局 db() 函数的行为是不一样的。Products Model 里有一处用了 $this->db('product_type')，在某些场景下会报错。改成 db('product_type') 就好了。
总结
接手老项目最怕的不是功能复杂，而是到处都是这种小坑。一个一个看起来都不严重，但加在一起就很头疼。
我的建议：
- 拿到项目第一件事，全局搜
->delete()，检查是否应该用软删除 - 搜所有
insert_system_log，确认日志内容和实际操作一致 - 检查所有
input()和方法参数，看有没有直接拼 SQL 或当表名用的 - 前台接口必须加状态和删除标记的过滤条件
- static 变量在 PHP 里是请求级别的，用的时候要注意重置
希望对接手老项目的你有所帮助。
评论
暂无评论