-
Notifications
You must be signed in to change notification settings - Fork 4
Enhancements to Mostly Static Resources (CFM-62) #442
base: develop
Are you sure you want to change the base?
Enhancements to Mostly Static Resources (CFM-62) #442
Conversation
5aa1e60 to
a9f042d
Compare
|
The force-push was to rebase against the latest develop. |
a9f042d to
bdf81de
Compare
app/config/app.php
Outdated
| // Allow caching: since the introduction of Mostly Static Resources, v5.3.0 | ||
| 'ini' => [ | ||
| 'session.cache_limiter' => '', | ||
| ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find this in the documentation, are you sure this is the write key and that it's still supported? (I see cacheLimiter referenced in Cake 2.)
Also, the comment "allow caching" doesn't semantically match what Cake 2 says this would do (define the cache control headers) and implies you're trying to turn on application level caching, which I don't think is what you're trying to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a legitimate way to do this. See: https://book.cakephp.org/5.x/development/sessions.html#setting-ini-directives
I agree that the comment isn't very exact. I can update the comment to something like:
// Prevent PHP's session handling from forcing no-cache headers onto every
// response, so actions like MostlyStaticResourcesController::deliver()
// can set their own explicit HTTP caching headers (since v5.3.0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inline code comment has been updated to that shown above.
| // lowercase alphanumeric characters and dashes | ||
| // lowercase alphanumeric characters, dashes, underscores, and periods (for allowing extensions) | ||
| $validator->add('name', [ | ||
| 'slugfilter' => [ | ||
| 'rule' => ['custom', '/^[a-z0-9-]+$/'], | ||
| 'rule' => ['custom', '/^[a-z0-9._\-]+$/'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me uncomfortable, though I'm not sure I have a good reason other than it's deviating from the out of the box pattern.
Cake 3+ uses dashes, not underscores, in URLs. I get that someone might upload a file with an underscore, but we don't need to preserve the filename exactly as uploaded.
Allowing periods for extensions also allows them for poorly constructed filenames. But maybe that's not a problem we should really be solving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My goal here was to allow filenames to upload with minimal transformation - and dots and underscores are fairly common in image filenames (I much prefer dashes). I'm ok with skipping underscores or enforcing their change to dashes if we feel strongly about this.
On extensions: we may not require them for rendering, since Content-Type is set explicitly from the stored mime_type column. But keeping the name field recognizable/familiar to whoever's managing these resources through the UI feels like better UX even if just a convenience. I'm hesitant to strip the extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'd prefer to preserve these - but I can compromise on the underscore. I feel more strongly about the dot (for extensions). Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, in the spirit of unnecessary compromise, let's convert underscores to dashes but leave the dots.
| @@ -894,7 +903,7 @@ msgid "MostlyStaticResources.name" | |||
| msgstr "Slug" | |||
|
|
|||
| msgid "MostlyStaticResources.name.desc" | |||
| msgstr "The URL fragment for this Resource, which must be unique and use only lowercase alphanumeric characters and dashes (-)" | |||
| msgstr "The URL fragment for this Resource, which must be unique and use only lowercase alphanumeric characters, dashes (-), underscores (_), and periods (.)" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See discussion in MostlyStaticResourcesTable, below.
bdf81de to
6e19811
Compare
|
Updates made (per comments above), and this PR has been rebased against the latest develop. |
| $builder->registerMiddleware('postMaxSizeCheck', new \App\Middleware\PostMaxSizeCheckMiddleware()); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not specific to this PR, but it looks like these registerMiddleware statements are supposed to be defined outside of any scope?
| $builder->connect( | ||
| '/add', | ||
| ['controller' => 'MostlyStaticResources', 'action' => 'add']); | ||
| $builder->connect( | ||
| '/edit/{id}', | ||
| ['controller' => 'MostlyStaticResources', 'action' => 'edit'], | ||
| ['id' => '\d+', 'pass' => ['id']] | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we inherit this from the default routes? ie, why do we need these and not delete?
| @@ -931,11 +934,17 @@ msgstr "This Petition is complete and has been finalized. Please contact your ad | |||
| msgid "MostlyStaticResources.file_content" | |||
| msgstr "File to Upload" | |||
|
|
|||
| msgid "MostlyStaticResources.image" | |||
| msgstr "Resource Image" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there are reason not just to use "Image"? (Similarly for "Resource URL".)
| return $handler->handle($request); | ||
| } | ||
|
|
||
| protected function iniSizeToBytes(string $val): int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just use PHP's ini_parse_quantity function instead? This imposes a minimum PHP of 8.2.0, but I think we're there already.
| // lowercase alphanumeric characters and dashes | ||
| // lowercase alphanumeric characters, dashes, underscores, and periods (for allowing extensions) | ||
| $validator->add('name', [ | ||
| 'slugfilter' => [ | ||
| 'rule' => ['custom', '/^[a-z0-9-]+$/'], | ||
| 'rule' => ['custom', '/^[a-z0-9._\-]+$/'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, in the spirit of unnecessary compromise, let's convert underscores to dashes but leave the dots.
This PR does a number of things:
Here is the edit view:
