Skip to content

Studio: Filter out deprecated warnings from fatal errors#3018

Merged
katinthehatsite merged 7 commits intotrunkfrom
fix/pushing-site-with-8.5
Apr 9, 2026
Merged

Studio: Filter out deprecated warnings from fatal errors#3018
katinthehatsite merged 7 commits intotrunkfrom
fix/pushing-site-with-8.5

Conversation

@katinthehatsite
Copy link
Copy Markdown
Contributor

Related issues

Related to STU-1529

How AI was used in this PR

It was used to identify the solution after the fix raised in Automattic/wp-cli-sqlite-command#24 uncovered the issue.

Proposed Changes

PHP 8.5 triggers a deprecation notice from react/promise inside wp-cli.phar. Studio treats any stderr output as a fatal export failure, so the notice was causing the export to fail even though the actual export succeeded. This PR ensures that we filter out the deprecation warning from the actual fatal error:

Error invoking remote method 'exportSiteForPush': Error: Database export      
  failed: PHP Deprecated: Case statements followed by a semicolon (;) are       
  deprecated, use a colon (:) instead in                                        
  phar:///tmp/wp-cli.phar/vendor/react/promise/src/functions.php on line 369    

This would ideally need to be fixed upstream but I think this fix should be sufficient for now.

Testing Instructions

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@katinthehatsite katinthehatsite self-assigned this Apr 8, 2026
@katinthehatsite katinthehatsite requested a review from a team April 8, 2026 18:24
@wpmobilebot
Copy link
Copy Markdown
Collaborator

wpmobilebot commented Apr 8, 2026

📊 Performance Test Results

Comparing 9dec3c2 vs trunk

app-size

Metric trunk 9dec3c2 Diff Change
App Size (Mac) 1275.78 MB 1275.78 MB 0.00 MB ⚪ 0.0%

site-editor

Metric trunk 9dec3c2 Diff Change
load 1908 ms 1861 ms 47 ms ⚪ 0.0%

site-startup

Metric trunk 9dec3c2 Diff Change
siteCreation 8144 ms 9123 ms +979 ms 🔴 12.0%
siteStartup 4176 ms 4313 ms +137 ms 🔴 3.3%

Results are median values from multiple test runs.

Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff)

Copy link
Copy Markdown
Contributor

@fredrikekelund fredrikekelund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now have two importer/exporter implementations: one in apps/cli and another in apps/studio.

In apps/cli, we only throw if WP-CLI returned a non-zero exit code, which I believe is the correct way to handle errors there.

I say let's do the same thing here.

@katinthehatsite
Copy link
Copy Markdown
Contributor Author

I say let's do the same thing here.

Checking now 👀

@katinthehatsite
Copy link
Copy Markdown
Contributor Author

@fredrikekelund the issue should now be addressed, let me know if you have any further comments.

Copy link
Copy Markdown
Contributor

@fredrikekelund fredrikekelund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 I'd probably make the equivalent change in apps/studio/src/lib/import-export/export/exporters/default-exporter.ts, too. Full-site exports already work when I test it with a PHP 8.5 site, but this change would ensure correctness.

@katinthehatsite
Copy link
Copy Markdown
Contributor Author

LGTM 👍 I'd probably make the equivalent change in apps/studio/src/lib/import-export/export/exporters/default-exporter.ts, too. Full-site exports already work when I test it with a PHP 8.5 site, but this change would ensure correctness.

Should now be done 👍

@katinthehatsite katinthehatsite merged commit b35987e into trunk Apr 9, 2026
10 checks passed
@katinthehatsite katinthehatsite deleted the fix/pushing-site-with-8.5 branch April 9, 2026 18:50
katinthehatsite added a commit that referenced this pull request Apr 9, 2026
* Filter out deprecated warnings from fatal errors

* Ensure our error filtering approach is consistent with CLI

* Apply changes to default exporter

* Fix unit tests

---------

Co-authored-by: Kateryna Kodonenko <kateryna@automattic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants