Conversation
|
@liseli Let's set this to draft since we won't be able to deploy it until production is all php 8. I would expect that will be in the next few weeks, so we shouldn't have to sit on this a very long time. |
It is done! |
moseshll
left a comment
moseshll
left a comment
There was a problem hiding this comment.
Everything looks as I would expect, and I had no issues running this locally.
There is one thing that I have been finding helpful, that may make sense to do here, is quiet down the overly chatty nginx rewrite messages that show up in the log. Each request results in a page of "did not match" messages for each line in the rewrite rules. We rarely need to debug these, so I do not think they are helpful to enable by default. This is what I changed in docker/nginx-default.conf:
rewrite_log on;->rewrite_log off;error_log /dev/stdout debug;->error_log /dev/stdout notice;
... and restart nginx service, see if navigating the site produces less "junk" in the log.
This is just a casual recommendation!
APPROVE
aelkiss
left a comment
aelkiss
left a comment
There was a problem hiding this comment.
Generally this all looks good and works as I expect locally in development. A couple things to consider mentioned in the comments.
| "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> | ||
| <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en"> | ||
| <title>{$doc.titles[0]} (OCLC {', '|implode:$doc.oclcs })</title> | ||
| <title>{$doc.titles[0]} (OCLC {$doc.oclcs|join:', '})</title> |
There was a problem hiding this comment.
This line gives a complaint about empty array if nothing matches (i.e. $doc.titles is empty). Not a new issue but probably worth fixing.
| return; | ||
| } | ||
|
|
||
| foreach (glob(rtrim($directory, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR . '*.php') as $file) { |
There was a problem hiding this comment.
Might be worth specifically enumerating what we're actually using rather than using the glob here, especially as it's not terribly likely to change in the future
|
re: turning off nginx rewrite rule debugging - agreed, this might have been an accidental commit at some point. |
3 participants
This PR focuses on migrating the Catalog template from Smarty 4 to Smarty 5.
As part of this PR, I've updated the template documentation