| @@ -53,13 +53,16 @@ | ||
| 53 | 53 | * agent_fixable:array<int,array<string,mixed>>, |
| 54 | 54 | * human_fixable:array<int,array<string,mixed>> |
| 55 | 55 | * } |
| 56 | 56 | */ |
| 57 | - public static function build( array $settings ): array { | |
| 57 | + public static function build( array $settings, ?array $score = null ): array { | |
| 58 | 58 | $opportunities = self::routed_opportunities( $settings ); |
| 59 | 59 | |
| 60 | 60 | return array( |
| 61 | - 'score' => self::latest_score(), | |
| 61 | + // A caller that already measured passes its result in rather than | |
| 62 | + // letting this re-read history — that is what makes a post-run | |
| 63 | + // score describe the site the run just changed. (#306) | |
| 64 | + 'score' => null === $score ? self::latest_score() : $score, | |
| 62 | 65 | 'agent_fixable' => array_merge( self::agent_fixable( $settings ), $opportunities['agent'] ), |
| 63 | 66 | 'human_fixable' => array_merge( self::human_fixable(), $opportunities['human'] ), |
| 64 | 67 | ); |
| 65 | 68 | } |
| @@ -216,17 +219,197 @@ | ||
| 216 | 219 | ); |
| 217 | 220 | } |
| 218 | 221 | |
| 219 | 222 | /** |
| 223 | + * How old a stored score may be before it is worth spending a measurement. | |
| 224 | + * | |
| 225 | + * Six hours. Short enough that a score quoted after an optimize run | |
| 226 | + * reflects the site as it is now, long enough that the repeated calls an | |
| 227 | + * assistant makes while working on one site reuse a single measurement | |
| 228 | + * rather than one each. | |
| 229 | + */ | |
| 230 | + public const STALE_AFTER = 6 * HOUR_IN_SECONDS; | |
| 231 | + | |
| 232 | + /** | |
| 233 | + * Shortest gap between two measurements this class will start. | |
| 234 | + * | |
| 235 | + * The cooldown, not the staleness window, is what bounds spend. An agent | |
| 236 | + * that calls optimize_site in a loop would otherwise start a PSI run per | |
| 237 | + * call and drain the account's quota in a minute — the exact cost the | |
| 238 | + * original "read from history" comment was written to avoid. Staleness | |
| 239 | + * decides whether a fresh number is WANTED; this decides whether one is | |
| 240 | + * ALLOWED. | |
| 241 | + */ | |
| 242 | + public const MEASURE_COOLDOWN = 15 * MINUTE_IN_SECONDS; | |
| 243 | + | |
| 244 | + /** Transient holding the timestamp of the last measurement we started. */ | |
| 245 | + private const COOLDOWN_KEY = 'xspeed_optimize_measured_at'; | |
| 246 | + | |
| 247 | + /** | |
| 248 | + * Measure now, if a measurement is both wanted and affordable. | |
| 249 | + * | |
| 250 | + * Returns the same shape as `latest_score()` so callers can treat a fresh | |
| 251 | + * and a stored score identically — the difference is visible in | |
| 252 | + * `age_seconds`, never in the structure. | |
| 253 | + * | |
| 254 | + * A failure here is deliberately NOT fatal. A missing API key, a quota | |
| 255 | + * refusal or a timeout means we fall back to the stored score, which is | |
| 256 | + * the behaviour this method replaced; a run that would have succeeded must | |
| 257 | + * not start failing because the score service is down. | |
| 258 | + * | |
| 259 | + * @param bool $force Skip the staleness check AND the cooldown — an | |
| 260 | + * explicit "measure now" from | |
| 261 | + * `--measure-score=always` or a multi-round | |
| 262 | + * tuner. Requires `score.enabled`: nothing | |
| 263 | + * bypasses that. | |
| 264 | + * @param bool $assume_stale Skip only the staleness check, keeping the | |
| 265 | + * cooldown. For the post-apply path, where | |
| 266 | + * changes just landed so the stored score is | |
| 267 | + * known to describe a site that no longer | |
| 268 | + * exists — but the caller did not ask to spend | |
| 269 | + * a measurement. Passing $force there billed | |
| 270 | + * every applying run against the quota the | |
| 271 | + * cooldown exists to protect. (#306 QA issue 1) | |
| 272 | + * @return array<string,mixed>|null | |
| 273 | + */ | |
| 274 | + public static function measure_fresh( bool $force = false, bool $assume_stale = false ): ?array { | |
| 275 | + $stored = self::latest_score(); | |
| 276 | + | |
| 277 | + if ( ! $force && ! $assume_stale && is_array( $stored ) && empty( $stored['stale'] ) ) { | |
| 278 | + return $stored; | |
| 279 | + } | |
| 280 | + | |
| 281 | + if ( ! class_exists( '\XSpeed\Score' ) ) { | |
| 282 | + return $stored; | |
| 283 | + } | |
| 284 | + | |
| 285 | + $opts = Settings_Manager::get( 'score' ); | |
| 286 | + | |
| 287 | + // External scores are OFF unless the site owner turned them on, and | |
| 288 | + // that is the only thing standing between this plugin and a third | |
| 289 | + // party. readme.txt promises "if the feature is left off — which is | |
| 290 | + // the default — no request is ever made", and the dashboard repeats | |
| 291 | + // it on screen. | |
| 292 | + // | |
| 293 | + // This call site read `psi_api_key`, `test_url` and `default_strategy` | |
| 294 | + // out of the Score settings and then never looked at `enabled`, so an | |
| 295 | + // optimize run sent the site's address to Google on a site configured | |
| 296 | + // never to contact anyone — including sites set up for GTmetrix. Every | |
| 297 | + // other PSI caller checks it (ScoreModule::rest_run, | |
| 298 | + // ScoreModule's CLI). (QA #306, issue 1) | |
| 299 | + if ( empty( $opts['enabled'] ) ) { | |
| 300 | + return $stored; | |
| 301 | + } | |
| 302 | + | |
| 303 | + // Measure with the provider the OWNER chose, or not at all. | |
| 304 | + // | |
| 305 | + // This method only knows how to run PSI, which answers synchronously. | |
| 306 | + // GTmetrix does not: start_gtmetrix() returns a pending marker and the | |
| 307 | + // result arrives later via poll_gtmetrix(), so there is no score to | |
| 308 | + // hand back inside one run. Ignoring the setting meant a site | |
| 309 | + // configured for GTmetrix — with no Google key at all — still had its | |
| 310 | + // address sent to Google, and that PSI result then became the headline | |
| 311 | + // score on the dashboard, displacing the GTmetrix history the owner | |
| 312 | + // set up. Every other part of the plugin honours `provider`; this call | |
| 313 | + // site never read it. (#306 QA issue 2) | |
| 314 | + // | |
| 315 | + // Falling back to the stored score is the same degradation this method | |
| 316 | + // already applies to a refused or failed measurement: the run still | |
| 317 | + // reports a number, with its age attached, and says nothing it cannot | |
| 318 | + // support. | |
| 319 | + // Skip only on an AFFIRMATIVE non-PSI choice. `??` alone catches null | |
| 320 | + // but not an empty string, `false`, or a case variant — and a blank | |
| 321 | + // `provider` in the option row (a hand-edited row, a partial | |
| 322 | + // migration, an older schema) would then disable post-run measurement | |
| 323 | + // permanently, with no diagnostic. Unset or empty means "the default", | |
| 324 | + // and the default is PSI. | |
| 325 | + $provider = strtolower( trim( (string) ( $opts['provider'] ?? '' ) ) ); | |
| 326 | + if ( '' !== $provider && 'psi' !== $provider ) { | |
| 327 | + return $stored; | |
| 328 | + } | |
| 329 | + | |
| 330 | + // The cooldown stops two runs racing into a paid measurement, but it | |
| 331 | + // must not silence an EXPLICIT request. `$force` is what | |
| 332 | + // `--measure-score=always` and a multi-round tuner send, and applying | |
| 333 | + // the full 15 minutes there meant round 2 reused round 1's number | |
| 334 | + // while the summary said "measured just now" — so a tuner planned its | |
| 335 | + // next round from before-the-first-round data and concluded its own | |
| 336 | + // changes had achieved nothing. (QA #306, issue 2) | |
| 337 | + // | |
| 338 | + // The quota protection is not traded away, because `$force` is not | |
| 339 | + // something a caller drifts into: it comes only from | |
| 340 | + // `--measure-score=always` — a person or a tuner saying "measure now" | |
| 341 | + // on purpose. The default path (`auto`) still waits the full cooldown, | |
| 342 | + // which is what an agent looping on optimize_site actually hits. | |
| 343 | + if ( ! $force && get_transient( self::COOLDOWN_KEY ) ) { | |
| 344 | + return $stored; | |
| 345 | + } | |
| 346 | + | |
| 347 | + $api_key = (string) ( $opts['psi_api_key'] ?? '' ); | |
| 348 | + | |
| 349 | + /** | |
| 350 | + * Filter whether an optimize run may start a fresh measurement. | |
| 351 | + * | |
| 352 | + * Without a key PSI still answers, but on a shared unauthenticated | |
| 353 | + * quota that a busy host can exhaust for everyone on the IP. Sites | |
| 354 | + * that would rather never spend a measurement here can return false. | |
| 355 | + * | |
| 356 | + * @since 1.2.0 | |
| 357 | + * | |
| 358 | + * @param bool $allowed Whether to measure. | |
| 359 | + * @param string $api_key The configured PSI key, empty if none. | |
| 360 | + */ | |
| 361 | + if ( ! apply_filters( 'xspeed_optimize_may_measure', true, $api_key ) ) { | |
| 362 | + return $stored; | |
| 363 | + } | |
| 364 | + | |
| 365 | + // Set the cooldown BEFORE the request, not after. PSI takes up to a | |
| 366 | + // minute; two calls arriving inside that window would both see no | |
| 367 | + // transient and both spend a measurement. | |
| 368 | + set_transient( self::COOLDOWN_KEY, time(), self::MEASURE_COOLDOWN ); | |
| 369 | + | |
| 370 | + // Measure what the site is configured to measure. Both this and the | |
| 371 | + // Score module's own runs write to the SAME history, so hardcoding | |
| 372 | + // the home page on mobile filed a result the dashboard then showed | |
| 373 | + // as the site's latest score — silently replacing the desktop or | |
| 374 | + // custom-URL audit the owner had set up (#306 review, issue 3). | |
| 375 | + $test_url = trim( (string) ( $opts['test_url'] ?? '' ) ); | |
| 376 | + if ( '' === $test_url ) { | |
| 377 | + $test_url = (string) home_url( '/' ); | |
| 378 | + } | |
| 379 | + | |
| 380 | + $strategy = (string) ( $opts['default_strategy'] ?? 'mobile' ); | |
| 381 | + if ( ! in_array( $strategy, array( 'mobile', 'desktop' ), true ) ) { | |
| 382 | + $strategy = 'mobile'; | |
| 383 | + } | |
| 384 | + | |
| 385 | + $row = Score::run_psi( $test_url, $strategy, $api_key ); | |
| 386 | + | |
| 387 | + if ( empty( $row['ok'] ) ) { | |
| 388 | + return $stored; | |
| 389 | + } | |
| 390 | + | |
| 391 | + return self::latest_score(); | |
| 392 | + } | |
| 393 | + | |
| 394 | + /** | |
| 220 | 395 | * The most recent stored audit, reduced to the numbers that decide a score. |
| 221 | 396 | * |
| 222 | - * Read from history rather than run fresh: an optimize run should not | |
| 223 | - * silently spend someone's PageSpeed allowance, and a score from this | |
| 224 | - * morning is enough to say what is wrong. | |
| 397 | + * Reads history only — it never measures. That was once the whole policy, | |
| 398 | + * on the reasoning that an optimize run should not silently spend someone's | |
| 399 | + * PageSpeed allowance and a score from this morning is enough to say what | |
| 400 | + * is wrong. The first half still holds and is why `measure_fresh()` is | |
| 401 | + * cooldown-bound rather than unconditional. The second half did not: on a | |
| 402 | + * site whose last audit was two weeks old, a run applied changes and then | |
| 403 | + * quoted the old number as its result, which reads as a claim the run had | |
| 404 | + * produced it. | |
| 225 | 405 | * |
| 406 | + * So this stayed the cheap path, `measure_fresh()` became the honest one, | |
| 407 | + * and `age_seconds` / `stale` here let every caller tell them apart. | |
| 408 | + * | |
| 226 | 409 | * @return array<string,mixed>|null |
| 227 | 410 | */ |
| 228 | - private static function latest_score(): ?array { | |
| 411 | + public static function latest_score(): ?array { | |
| 229 | 412 | if ( ! class_exists( '\XSpeed\Score' ) ) { |
| 230 | 413 | return null; |
| 231 | 414 | } |
| 232 | 415 | // Score::latest() is the newest run with ok === true. History is |
| @@ -251,13 +434,22 @@ | ||
| 251 | 434 | 'rating' => self::rate( $key, (float) $value ), |
| 252 | 435 | ); |
| 253 | 436 | } |
| 254 | 437 | |
| 438 | + // Age travels WITH the score, always. A number quoted without it is how | |
| 439 | + // a two-week-old 77 gets relayed as the result of a run that just | |
| 440 | + // finished — the reading is not wrong so much as unanswerable, because | |
| 441 | + // nothing in the payload said when it was true. | |
| 442 | + $ran_at = isset( $latest['ts'] ) && is_numeric( $latest['ts'] ) ? (int) $latest['ts'] : null; | |
| 443 | + $age = null === $ran_at ? null : max( 0, time() - $ran_at ); | |
| 444 | + | |
| 255 | 445 | return array( |
| 256 | - 'score' => $latest['score'] ?? null, | |
| 257 | - 'strategy' => $latest['strategy'] ?? null, | |
| 258 | - 'ran_at' => $latest['ts'] ?? null, | |
| 259 | - 'metrics' => $metrics, | |
| 446 | + 'score' => $latest['score'] ?? null, | |
| 447 | + 'strategy' => $latest['strategy'] ?? null, | |
| 448 | + 'ran_at' => $ran_at, | |
| 449 | + 'age_seconds' => $age, | |
| 450 | + 'stale' => null === $age || $age > self::STALE_AFTER, | |
| 451 | + 'metrics' => $metrics, | |
| 260 | 452 | ); |
| 261 | 453 | } |
| 262 | 454 | |
| 263 | 455 | /** |
