Skip to content

Only write the legacy routing alias cache when caching is enabled - #554

Closed
buyolitsez wants to merge 2 commits into
BeWelcome:developfrom
buyolitsez:502
Closed

buyolitsez wants to merge 2 commits into
BeWelcome:developfrom
buyolitsez:502

Conversation

@buyolitsez

Copy link
Copy Markdown
Contributor

Refs #502

Problem

Every legacy page load logs a file_put_contents(build/alias.cache.ini): Failed to open stream warning.

  • rox_docker.ini sets IniCache = "0". That makes $force_refresh always true in RequestRouter::loadRoutingAliasTable(), so the cache file is never read.
  • The router still calls iniWrite() on every request.
  • build/ belongs to root in the image and PHP-FPM runs as www-data, so the write fails.

Changes

tools/routing/requestrouter.class.php: only write the alias cache when caching is actually on, i.e. wrap iniWrite() in if (!$force_refresh). This is option D from the issue.

  • With IniCache = 0 (or on localhost), nothing is written and routing behaves as before.
  • With IniCache = 1, the cache is still written and read as before.

Testing

New tests/Tools/RequestRouterTest.php, a plain unit test with no kernel or DB. It covers caching disabled, localhost and caching enabled. On develop the disabled and localhost cases fail.

Notes

The issue suggests a PR on feature/docker-master. This patch applies cleanly there too, so I can open a second PR against it if you prefer.

@thisismeonmounteverest

Copy link
Copy Markdown
Contributor

Setting IniCache to 1 fixes the recreation of the file as long as the file exists. The generated file alias.cache.ini has been stale for years now, just commiting the file and updating the rox_default.ini should do the trick. No need for code changes or unit tests that break depending on where they are run.

@thisismeonmounteverest

Copy link
Copy Markdown
Contributor

Fixed with #610.

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.

2 participants