[pkp-lib][main] #10357 Allow null contextId for emailTemplate Repository #10358
Conversation
| }) | ||
|
|
||
| ->when(!is_null($this->keys), function (Builder $q) { | ||
| ->selectRaw($this->contextId !== null ? $this->contextId . ' as context_id' : 'NULL as context_id') |
There was a problem hiding this comment.
I'd rather avoid string concatenation of queries as much as possible; Jonas had an approach that we should also follow for consistency. See:
pkp-lib/classes/userGroup/Collector.php
Line 302 in 1b6f1b5
We risk confusing null (constraint not specified to the collector) with SITE_CONTEXT_ID (used to be 0, now null). In cases like this it's better to have the caller provide a list of context IDs, with the SITE_ID_CONTEXT constant being one of the available options. Then, per the link above, map the array to intval when the query is being built and use COALESCE(context_id, 0) in the SQL in order to map NULLs to 0 there.
The idea is that calling code can use:
$collector->filterByContextIds([$journal->getId()]); // Filter by a single journal ID -- note the array syntax
...or...
$collector->filterByContextIds(null); // Do not filter by context IDs -- clear any pre-existing filter
...or...
$collector->filterByContextIds([SITE_ID_CONTEXT]); // Filter for templates that are site-wide -- note the array syntax
The need to specify the context ID in the constructor in this class is non-standard. I'd suggest not requiring any filters in the constructor, and instead using fluent functions to chain constraints like we do elsewhere.
There was a problem hiding this comment.
@asmecher, thank you for the feedback. I understand the importance of avoiding query string concatenation and aligning with Jonas’s approach for consistency. Here’s my take on handling contextId in email templates:
I believe that when we’re dealing with emailTemplates, we are specifically referring to context-level email templates. The user, as far as I know, cannot modify the default email template at the Site level. This means that the email_templates table shouldn't typically contain rows with null in the context_id column.
Given this, we seem to have two primary cases to consider:
-
When
contextIdis provided: In this case, we would join theemail_templatestable within theCollector's query to retrieve data specific to that context. -
When
contextIdis not provided: Here, we would useApplication::SITE_CONTEXT_IDas thecontextId. In this scenario, we retrieve default data fromemail_templates_default_data, bypassing any need forcontextIdin theCollector's query.
The root of this issue lies in the change of Application::SITE_CONTEXT_ID from 0 to null to align with updated database standards and ensure consistency with other cases. This shift led to back-office issues, as contextId was previously non-nullable in both Repo and Collector. Consequently, we needed to adjust the code to handle null values correctly.
However, while we could potentially introduce a specific Repo function to handle Site-level email templates, I believe that would be more appropriate as part of a broader future task rather than within the scope of this issue.
No description provided.