fix(upgrade): correct more of what real 1.x extensions' upgrades got wrong

boilerplate/skeleton/extension/js/package.json

@@ -10,7 +10,7 @@
     "@flarum/jest-config": "^2.0.0",
     "@flarum/prettier-config": "^1.0.0",
     "flarum-tsconfig": "^2.0.0",
-    "flarum-webpack-config": "^3.0.6",
+    "flarum-webpack-config": "^3.0.7",
     "prettier": "^2.5.1",
     "typescript": "^4.9.5",
     "typescript-coverage-report": "^0.6.1",

php-subsystem/src/NodeVisitors/AddExtender.php

@@ -28,9 +28,57 @@ public function __construct($params, NameContext $nameContext)
     $this->nameContext = $nameContext;
   }
 
+  /**
+   * The return statements whose array of extenders gets the new one.
+   *
+   * @var array<int, true>
+   */
+  protected $targets = [];
+
+  /**
+   * Picks the arrays of extenders the file returns: those returned outside any
+   * function, or failing that, from the fewest nested functions. An array a
+   * Conditional's closure returns is left alone, as it holds the extenders for
+   * when another extension is enabled.
+   */
+  public function beforeTraverse(array $nodes)
+  {
+    $returns = [];
+
+    $find = function (array $nodes, int $depth) use (&$find, &$returns) {
+      foreach ($nodes as $node) {
+        if (! $node instanceof Node) {
+          continue;
+        }
+
+        if ($node instanceof Node\Stmt\Return_ && $node->expr instanceof Node\Expr\Array_) {
+          $returns[] = [$node, $depth];
+        }
+
+        $nested = $node instanceof Node\FunctionLike ? $depth + 1 : $depth;
+
+        foreach ($node->getSubNodeNames() as $name) {
+          $find(is_array($node->$name) ? $node->$name : [$node->$name], $nested);
+        }
+      }
+    };
+
+    $find($nodes, 0);
+
+    $outermost = min(array_column($returns, 1) ?: [0]);
+
+    foreach ($returns as [$return, $depth]) {
+      if ($depth === $outermost) {
+        $this->targets[spl_object_id($return)] = true;
+      }
+    }
+
+    return null;
+  }
+
   public function leaveNode(Node $node)
   {
-    if ($node instanceof Node\Stmt\Return_ && $node->expr instanceof Node\Expr\Array_) {
+    if ($node instanceof Node\Stmt\Return_ && $node->expr instanceof Node\Expr\Array_ && isset($this->targets[spl_object_id($node)])) {
         if (! isset($this->params['extender'])) {
             $params = json_encode($this->params);
             throw new \Exception("Invalid Extender Schema. Provided params: $params");

php-subsystem/src/Upgrade/TwoPointOh/Search.php

@@ -230,6 +230,39 @@ public function __construct($returnExtendersNode)
                     $this->returnExtendersNode = $returnExtendersNode;
                 }
 
+                /** The class a registration names, by the name it has once it is a filter. */
+                private function registeredClass(Node\Expr $expr): ?string
+                {
+                    if ($expr instanceof Node\Expr\ClassConstFetch && $expr->class instanceof Node\Name) {
+                        $name = ($expr->class->getAttribute('resolvedName') ?? $expr->class)->toString();
+                    } elseif ($expr instanceof Node\Scalar\String_) {
+                        $name = $expr->value;
+                    } else {
+                        return null;
+                    }
+
+                    $name = str_replace(['FilterGambit', 'GambitFilter'], 'Filter', $name);
+
+                    return ltrim(str_replace('Gambit', 'Filter', $name), '\\');
+                }
+
+                /**
+                 * 1.x registered a class that was both a gambit and a filter
+                 * twice, with each extender: 2.0 needs it once.
+                 */
+                private function register(string $searcher, string $kind, Node\Expr $expr): void
+                {
+                    $class = $this->registeredClass($expr);
+
+                    foreach ($this->data['searchers'][$searcher][$kind] ?? [] as $registered) {
+                        if ($class !== null && $this->registeredClass($registered) === $class) {
+                            return;
+                        }
+                    }
+
+                    $this->data['searchers'][$searcher][$kind][] = $expr;
+                }
+
                 private function findNew(Node $node): ?Node\Expr\New_
                 {
                     if ($node instanceof Node\Expr\New_) {
@@ -292,9 +325,9 @@ public function leaveNode(Node $node)
                             }
 
                             if ($method === 'addFilter') {
-                                $this->data['searchers'][$searcher]['filters'][] = $arg->value;
+                                $this->register($searcher, 'filters', $arg->value);
                             } elseif ($method === 'addFilterMutator') {
-                                $this->data['searchers'][$searcher]['mutators'][] = $arg->value;
+                                $this->register($searcher, 'mutators', $arg->value);
                             }
                         } elseif ($class === 'Flarum\Extend\SimpleFlarumSearch') {
                             $searcher = NodeUtil::className($new->args[0]->value->class);
@@ -318,7 +351,7 @@ public function leaveNode(Node $node)
                             }
 
                             if ($method === 'addGambit') {
-                                $this->data['searchers'][$searcher]['filters'][] = $arg->value;
+                                $this->register($searcher, 'filters', $arg->value);
                             } elseif ($method === 'setFullTextGambit') {
                                 $this->data['searchers'][$searcher]['fulltext'] = isset($resolvedName)
                                     ? $resolvedName->name
@@ -381,13 +414,48 @@ public function __construct($visitor)
             $collectorData = $collector->data;
         } elseif (isset($visitor)) {
             $collectorData = [
-                'extendData' => $visitor->data,
+                'extendData' => $this->reportable($visitor->data),
             ];
         }
 
         return new ReplacementResult($code, $collectorData);
     }
 
+    /**
+     * The searchers the extenders registered, with each filter and mutator by
+     * the name it is written with, which is what the step reads. Not their
+     * syntax trees: a fully qualified name's refers to itself, which can't be
+     * encoded as JSON.
+     */
+    protected function reportable(array $data): array
+    {
+        $searchers = [];
+
+        foreach ($data['searchers'] ?? [] as $searcher => $registered) {
+            foreach (['filters', 'mutators'] as $kind) {
+                if (! isset($registered[$kind])) {
+                    continue;
+                }
+
+                $classes = [];
+
+                foreach ($registered[$kind] as $expr) {
+                    if ($expr instanceof Node\Scalar\String_) {
+                        $classes[] = ['class' => ['name' => $expr->value]];
+                    } elseif ($expr instanceof Node\Expr\ClassConstFetch && $expr->class instanceof Node\Name) {
+                        $classes[] = ['class' => ['name' => $expr->class->name]];
+                    }
+                }
+
+                $registered[$kind] = $classes;
+            }
+
+            $searchers[$searcher] = $registered;
+        }
+
+        return ['searchers' => $searchers];
+    }
+
     function replaceNamespaces(string $file, string $code, array $ast, array $data): ?ReplacementResult
     {
         $traverser = $this->traverser();
@@ -439,16 +507,32 @@ function changeSignatures(string $file, string $code, array $ast, array $data):
                     ],
                     'return' => ['void'],
                 ],
-                'constrain' => [
-                    'params' => [
-                        'query' => ['\\Illuminate\\Database\\Eloquent\\Builder'],
-                        'actor' => ['\\Flarum\\User\\User'],
-                        'negate' => ['bool'],
-                    ],
-                    'return' => ['void'],
-                ],
             ],
-        ]));
+        ], function (Node $node) {
+            // A filter's constrain() helper is its own, so its parameters are
+            // left as they are, but for the query it is given: 2.0 makes that
+            // an Eloquent builder.
+            if (! $node instanceof Node\Stmt\ClassMethod || strtolower($node->name->name) !== 'constrain') {
+                return;
+            }
+
+            foreach ($node->params as $param) {
+                $nullable = $param->type instanceof Node\NullableType;
+                $type = $nullable ? $param->type->type : $param->type;
+
+                if (! $type instanceof Node\Name || ltrim(NodeUtil::className($type), '\\') !== 'Illuminate\\Database\\Query\\Builder') {
+                    continue;
+                }
+
+                $eloquent = NodeUtil::makeType(['\\Illuminate\\Database\\Eloquent\\Builder']);
+
+                if ($nullable) {
+                    $param->type->type = $eloquent;
+                } else {
+                    $param->type = $eloquent;
+                }
+            }
+        }));
 
         return new ReplacementResult($traverser->traverse($ast));
     }

src/steps/upgrade/twopointoh/backend/search.ts

@@ -7,6 +7,8 @@ import { GenerateGambitStub } from '../../../stubs/frontend/gambit';
 import { GenerateSearchGambitExtender } from '../../../js/search-gambit';
 import { LocaleStep } from '../../../locale/base';
 import { pluralKebabCaseModel } from '../../../../utils/model-name';
+import fs from 'fs';
+import path from 'path';
 
 type Gambit = {
   name: string;
@@ -15,6 +17,9 @@ type Gambit = {
   filterFile: string;
 };
 
+/** A gambit with an is: pattern, such as is:private, rather than a key and value. */
+const isBoolean = (gambit: Gambit) => gambit.pattern.includes('is:');
+
 export default class Search extends BaseUpgradeStep {
   type = 'Search & Filter API changes';
   beforeHook = true;
@@ -136,12 +141,6 @@ export default class Search extends BaseUpgradeStep {
           const result = await this.command.runSteps(
             new StepManager<FlarumProviders>().silentGroup((steps) => {
               this.gambits.forEach((gambit, index) => {
-                let gambitKey = gambit.pattern.replace('is:', '');
-
-                if (gambitKey.includes(':')) {
-                  gambitKey = gambitKey.split(':')[0];
-                }
-
                 const relatedSearcher = Object.keys(this.data.collectedData.extendData.searchers).find((searcher) => {
                   const filters = this.data.collectedData.extendData.searchers[searcher].filters || [];
                   return filters.some((filter: any) => {
@@ -155,11 +154,11 @@ export default class Search extends BaseUpgradeStep {
                   type = pluralKebabCaseModel(relatedSearcher.split('\\').pop()!.replace('Searcher', ''));
                 }
 
-                steps
+                const gambitSteps = steps
                   .namedStep(`gambit-${index}`, new GenerateGambitStub(this.command.STUB_PATH, genExtScaffolder()), {}, [], {
                     frontend: 'common',
                     className: gambit.name,
-                    type: gambit.pattern.includes('is:') ? 'boolean' : 'key-value',
+                    type: isBoolean(gambit) ? 'boolean' : 'key-value',
                     filterKey: gambit.filterKey,
                   })
                   .step(
@@ -176,11 +175,11 @@ export default class Search extends BaseUpgradeStep {
                       frontend: 'common',
                       modelType: type,
                     }
-                  )
-                  .step(new LocaleStep(genExtScaffolder()), {}, [], {
-                    key: `lib.gambits.${gambit.filterKey}.key`,
-                    value: gambitKey,
-                  });
+                  );
+
+                this.gambitTranslations(gambit).forEach((translation) => {
+                  gambitSteps.step(new LocaleStep(genExtScaffolder()), {}, [], translation);
+                });
 
                 delete this.gambits[index];
               });
@@ -209,6 +208,31 @@ export default class Search extends BaseUpgradeStep {
     ];
   }
 
+  /**
+   * The translations a gambit's stub uses: its key, and a key-value gambit's
+   * hint, which the search shows beside it. Only the author can say what the
+   * hint should be, so it is left for them to write.
+   */
+  gambitTranslations(gambit: Gambit): { key: string; value: string }[] {
+    let gambitKey = gambit.pattern.replace('is:', '');
+
+    if (gambitKey.includes(':')) {
+      gambitKey = gambitKey.split(':')[0];
+    }
+
+    const translations = [{ key: `lib.gambits.${gambit.filterKey}.key`, value: gambitKey }];
+
+    if (!isBoolean(gambit)) {
+      const hint = `lib.gambits.${gambit.filterKey}.hint`;
+      const locale = fs.existsSync(path.join(this.root, 'resources/locale/en.yml')) ? 'resources/locale/en.yml' : 'locale/en.yml';
+
+      translations.push({ key: hint, value: gambitKey });
+      this.leaveTodo(locale, `describe what the ${gambitKey} gambit takes in ${hint}, which the search shows beside it.`);
+    }
+
+    return translations;
+  }
+
   targets(): string[] {
     return ['src/**/*', 'extend.php'];
   }

src/steps/upgrade/twopointoh/dependencies.ts

@@ -275,9 +275,10 @@ export default class Dependencies extends BaseUpgradeStep {
 
       // The versions a new extension is scaffolded with. 3.0.6 is the first
       // flarum-webpack-config that loads an extension's code-split chunks from
-      // its own build.
+      // its own build, and 3.0.7 the first that exports a default export as its
+      // value, not under a name taken from its code.
       const map: Record<string, string> = {
-        'flarum-webpack-config': '^3.0.6',
+        'flarum-webpack-config': '^3.0.7',
         'flarum-tsconfig': '^2.0.0',
         '@flarum/jest-config': '^2.0.0',
         webpack: '^5.104.1',

src/steps/upgrade/twopointoh/frontend/admin-extender.ts

@@ -4,9 +4,9 @@ import globby from 'globby';
 import chalk from 'chalk';
 import traverse, { Binding, NodePath } from '@babel/traverse';
 import * as t from '@babel/types';
-import { parse } from 'recast/parsers/babel-ts';
 import { BaseUpgradeStep, GitCommit, Replacement } from '../base';
 import { formatCode } from '../../../../utils/ast';
+import { parse } from '../../../../utils/babel-ts';
 import { extensionId } from '../../../../utils/composer';
 
 const TODO = '@TODO: register this with Extend.Admin in admin/extend instead';