Skip to content

Conversation

@AllenJB
Copy link
Contributor

@AllenJB AllenJB commented Nov 3, 2025

This PR is related to php/phd#212 and php/phd#204

IMPORTANT: At least one of the above 2 PRs to phd MUST be merged before this one.

These changes modify search.js to use the new pre-combined search indexes (significantly simplifying the code) added by both the above mentioned PRs.

Additionally the changes required for supporting the search on local docs (which set the language to local to trigger the alternative behavior)

The version of search.js used here is the same as that included in php/phd#204 . After this PR has been merged, phd can be modified to pull this copy of search.js instead of duplicating it in the phd repository, the same as it does for stylesheets.

@derickr derickr merged commit 658fafe into php:master Nov 4, 2025
3 checks passed
@AllenJB AllenJB deleted the search-combined-indexes branch November 5, 2025 21:59
@theodorejb
Copy link
Contributor

It seems like this broke the visual snapshot tests, since they are failing on all PRs after this was merged. Actual image:
actual

Expected image:
expected

@derickr
Copy link
Member

derickr commented Nov 16, 2025

@AllenJB I hadn't noticed this made the tests fail. Can you have a look?

@AllenJB
Copy link
Contributor Author

AllenJB commented Nov 16, 2025

@derickr I don't believe my changes caused these test failures.

Based on manual testing, the code is doing what it always does and correctly sending the user to /search.php?lang={language}&q={query}, but that page then changes the URL. I believe this is likely caused by the Google CSE scripts.

You can see this happen on php.net itself (in addition to testing with the php -S local setup)

My guess is that there's been a change in the Google CSE scripts causing this (hence why the tests passed when this PR was merged).

Based on the PR Preview Action Logs this occurred some time on Nov 4th between 11am and 8pm GMT (between runs for PRs 665 and 666)

My guess is the test needs to be updated to account for the Google CSE changes, but I'm not sure the best way to do that here.

@sy-records
Copy link
Member

@saundefined Could you take a look? I don't have server permissions. I suspect it might be an issue with the script?

SCRIPT_BEFORE: bash /home/thephpfoundation/scripts/pr_created_pre.sh web-php ${{ github.event.number }}
SCRIPT_AFTER: bash /home/thephpfoundation/scripts/pr_created.sh web-php ${{ github.event.number }}

The search-combined.json file wasn't copied?

@saundefined
Copy link
Member

@sy-records the search-combined.json is generated, and the search works on the preview boxes.
I'll try to see what's wrong with the tests 🤔

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.

5 participants