By the way, now that this fix ensures the system.performance.cache.page.max_age
config value is properly honored by PageCache
, I'm not sure how it will overlap with
🐛
[pp-3] Bubbling of elements' max-age to the page's headers and the page cache
Postponed
, but it's certainly related.
I've finally fixed the caching behavior and all tests.
Things I'm concerned about:
- I had to remove the weak (
W/
) prefix from theETag
header, as it was sometimes being added, causing multipleEntityResourceTestBase
tests to fail. I was able to reproduce the issue, but I'm not entirely sure if it's due to an Nginx configuration or something else. - Now that
system.performance.cache.page.max_age
is being properly honored, I had to set amax_age
value in several tests expecting responses to be cacheable. I'm not sure if this is the correct approach or if we should configure it globally inBrowserTestBase
. - I've assumed that having two
Vary
headers in the response is correct, but I haven't determined why they are being set separately. An additionalAccept-Encoding
value is added in a secondVary
header instead of being merged into the existingVary
header containingCookie
andOrigin
values. To account for this, I modifiedCorsIntegrationTest
to check for "Origin" across all possibleVary
headers.
After digging into the code, I realized I made a mistake. To prevent multiple tests from failing, I mistakenly kept the very piece of code that was wrong in the first place:
$expire = ($date > $request_time) ? $date : Cache::PERMANENT;
I also confirmed that this issue is only related to the Expires
header and not to PageCache’s behavior of ignoring max-age
. For that second issue, please refer to
🐛
[pp-3] Bubbling of elements' max-age to the page's headers and the page cache
Postponed
@johnv.
I have another idea, although it's not exactly in linew with @alexpott suggested:
What if we fire a dedicated event for state changes, such as StateKeySetEvent
, to which a MaintenanceModeEventSubscriber
could listen and log a more specific message?
We could dispatch the event before the key is updated, allowing subscribers to retrieve the original value directly from the state instead of passing it within the event, only in case they need it. This would introduce a performance impact, but only for subscribers that actually need to access the original value. Given that there would only be a few subscribers, the impact wouldn’t be as significant as retrieving the original value for every key set in a request.
What do you think?
I'm not feeling as confident with naming as I usually do while writing this code… maybe it's just late, and I'm not thinking clearly. 🙂
- I'm unsure about the
StateKeysSetLoggerSubscriber
subscriber name. - I'm also not completely sure about the service name
state.keys_set_logger
. - I feel uneasy about not being able to provide a more specific log message when maintenance mode is set, rather than just logging "The state key system.maintenance_mode has been changed from 0 to 1."
Nonetheless, I believe this approach addresses #60 and the concerns discussed earlier.
Any feedback would be greatly appreciated!
Sounds good. I'll work into this :)
Umm, this is getting interesting. Some thoughts:
- If
system.maintenance_mode
is set multiple times within the same request or CLI operation, the last change could be from 1 to 1, which wouldn’t be useful. However, that’s very unlikely to happen, though. - If you believe that the final value is what matters most, then storing the previous value isn’t relevant.
- On the other hand, if you consider the previous value important, I start to worry about an edge case like the one mentioned in point 1.
@smustgrave I've replied to your comments.
Thanks for the review!
Ow, yes, you're right. I was thinking about Drupal 12, but the CR is for the Drupal 11.x minor version where this issue will be merged.
Sorry, I'm yet a CR rookie! xD
I tried to clarify them a bit :)
Thanks!
Thanks! I've replied to your comment in the MR.
Addressed! :)
@jonathan_hunt, if you add the "public" protocol in a link field configuration (after applying the changes in this issue fork: https://git.drupalcode.org/issue/drupal-2031149) you will be able to add links like public://whatever/you/want/file.pdf
, but those URIs will not be converted to an URL. You will have to process them in a custom hook.
Maybe the module Linkit (
https://www.drupal.org/project/linkit →
) would address what you want? Although a relative path would be stored in the field, instead of a public://
style URI, the public directory is not likely to change, so it would be enough for you, depending on the case.
vidorado → changed the visibility of the branch 11.x to hidden.
This was a tough one, but I believe I've managed to sort things out.
Some unexpected behaviors were occurring, such as the FinishResponseSubscriber
setting an Expires
header, which made the response non-cacheable—inside the setResponseCacheable()
method, which should theoretically do the opposite.
Additionally, some functional tests in PageCacheTest
were incorrectly expecting the response to be non-cacheable when it actually should have been. As a result, they were effectively masking the issue with cacheability headers.
A more in-deep review is necessary, as this is a complex scenario with multiple edge cases involved, and a thorough understanding of the HTTP cacheability headers specification is required to do so.
Also, I haven't been able to mark the MR as "Ready" I'm not sure if someone with higher privileges could do it, so I don't have to create a new one.
vidorado → changed the visibility of the branch 11.x to hidden.
vidorado → made their first commit to this issue’s fork.
@joseph.olstad Perhaps what you expect is only valid for your specific use case?
When creating a node in French, the preview displays the original, not-yet-saved French translation of the node, and the tag is shown in the entity's language. Isn't that the expected behavior?
I believe we shouldn't suppress errors caused by a contrib or custom FieldFormatter
. The return type of FieldFormatterInterface::viewElements()
is explicitly defined as array
, so no formatter should return NULL
.
I would close this as "Works as designed."
Trying to get feedback
vidorado → created an issue.
I believe this is a significant change that could cause WSODs after an update. Instead of throwing an exception, we should trigger a deprecation error first.
I've also fixed some failing tests caused by a core route that defined a route requirement for a menu_link_content
entity—which has bundles—without specifying a bundle.
Additionally, a change record has been issued. I will create a follow-up issue to replace the deprecation error with an \InvalidArgumentException
.
vidorado → made their first commit to this issue’s fork.
The last MR commit was silencing the filesize test, which was failing after the fix, so I've restored it. Additionally, I've tested also that the file size is only validated over new files.
@kim.pepper, thanks for pointing the deprecation out. I've seen that it was already addressed in the existing MR commit.
vidorado → made their first commit to this issue’s fork.
First of all, congratulations to @seanb! The media library code was not trivial, and their fix was both well-executed and clean.
After manually testing, I noticed that the langcode
was not being properly set in the translation add form. To fix this, I modified the code to retrieve the langcode
from the form_state storage, which turned out to contain the target translation’s langcode.
Since the documentation states that "no specific support is provided for it in the Form API" regarding the FormState::storage
variable, I added a fallback to the original fix $entity's language, which is for sure always available:
$form_state->get('langcode') ?? $entity->language()->getId(),
Additionally, I’ve included an FJ test. :)
I've added test coverage.
As a bonus, it turns out that we've fixed, at least partially, a bug stated in
🐛
Domain-based language negotiation strips "destination" URL query argument, causing BigPipe error
Closed: duplicate
, so I have had to change a line in BlockUiTest
too.
vidorado → made their first commit to this issue’s fork.
vidorado → made their first commit to this issue’s fork.
I've added a kernel test to ensure that a single delete query is executed against both the data table and the revision data table.
I'm not sure if directly testing the protected deleteFromDedicatedTables()
method is the best approach, for the sake of simplicity, or if I should have mocked even more components to test it only through the public interface of SqlContentEntityStorage
.
vidorado → made their first commit to this issue’s fork.
Changing version to latest 10.x as I see in the IS that this is obsolete for 11.x
I've managed to move the _editor_get_entity_reference_revisions()
function to a helper service with unit test coverage.
Thanks for your feedback @smustgrave, I've addressed all your suggestions :)
Haha, I’m with you on being efficient and avoiding unnecessary duplication of resources.
We could refactor EntityCacheTagsTestBase so that the cache bins are stored in a class member variable within the setUp() method, which is a common practice. However, I always try to be cautious about not straying too far from the original scope.
You have a lot of experience, so I’m always a bit worried about overlooking something.
Let’s see if others can share their thoughts, and we’ll decide from there. :)
Thanks so much for all the effort you put into reviewing so many (often complex) issue queues!
So, are you suggesting that the "View own account details" permission should be granted to authenticated users by default? That could make sense, but it would also mean that users would suddenly see their roles appearing in their own user edit forms.
If that’s the intended behavior, an upgrade path should be implemented. However, I’m not entirely sure if that would be the best approach. What do you think?
@smustgrave I don’t see how users could suddenly lose their access. The only users who had view+edit access to roles in user edit forms were those with the "Administer permissions" permission, and that remains unchanged. As far as I understand, the only addition in this change is a read-only view of one's own roles for users with "View own account details" permission.
Regarding REST APIs, we’ve only added an AccessResult::allowed()
when the user has "View own account details", while keeping AccessResult::neutral()
otherwise, which aligns with the previous behavior.
Could you clarify where exactly you see a potential permission loss?
Thanks!
You're right, and now I've seen that a isset()
guard was added in
🐛
There is no way to delete file entities of other users
Fixed
after this issue was created, so this is not longer a problem. I think we can close this issue as outdated.
if ($account->id() == $file_uid[0]['target_id']) {
Was changed to:
if (isset($file_uid[0]['target_id']) && $account->id() == $file_uid[0]['target_id']) {
It was better to check for $entity->getOwnerId()
rather than $file_uid = $entity->get('uid')->getValue()
and $file_uid[0]['target_id']
, but I think that's now out of the scope of this issue.
vidorado → made their first commit to this issue’s fork.
Thanks for pointing it out. Now I see that \Drupal\Tests\file\Kernel\AccessTest
already tests the update operation over a file with and without owner, so I believe we have this covered! :) haven't we?
I've debugged step by step and checked that the new code is exercised.
...
$this->assertFalse($file1->access('update', $user_own));
$this->assertTrue($file2->access('update', $user_own));
...
// Create a file with no user entity.
// <<Creates $file4 with no uid>>
$this->assertFalse($file4->access('update', $user_own));
...
$this->assertFalse($file4->access('update', $user_any));
Trying to get feedback.
Sorry, I completely overlooked the second point in #18.
I've implemented it, covered it with a test, and also updated the issue title. I also took the opportunity to rebase the MR.
Fixed a deprecation and restoring to RTBC
The new numbers look reasonable to me
I could help you out if you want. Let me know here or contact me in slack :)
All threads are now resolved. Thanks.
Done!
I guess I can safely restore to RTBC...
@catch I realize now that I completely misunderstood the use of CSRF tokens. I thought they were also meant to be used for public links, ensuring they were generated by Drupal, similar to how the itok
parameter works for image styles.
After reviewing the code more closely, I see your point. A session-stored random seed is used to generate the CSRF token, so it doesn’t actually make sense to include it in a 'public' link.
class CsrfTokenGenerator {
...
public function get($value = '') {
$seed = $this->sessionMetadata->getCsrfTokenSeed();
if (empty($seed)) {
$seed = Crypt::randomBytesBase64();
$this->sessionMetadata->setCsrfTokenSeed($seed);
}
return $this->computeToken($seed, $value);
}
...
}
So I’ll revert that part. Thanks for pointing it out! :)
Thanks for your suggestions @longwave!
I'm responding to you in the MR on git.drupalcode.org instead of here in the issue queue, to avoid bothering other followers with detailed comments :)
I had to adjust my notification preferences on git.drupalcode.org because an internal email was selected for notifications instead of my actual email. Now, I’ll finally receive notifications properly.
Summary lines look good to me :)
All done! :)
You're right @nikolay_shapovalov! We forgot to update the screenshot after the UX review and changes :)
I've updated the screenshot and taken a quick look to yor MR comments, which look very useful to me. I will address everything as soon as possible.
Thank you very much!
Created a MR, applied the patch from #5 and added a kernel test.
vidorado → made their first commit to this issue’s fork.
Trying to get feedback
Trying to get feedback
The subject code in #1932652 was developed on August 2013 (although it was commited later). In that moment, the code setting the file as permanent in FileUsageBase::add()
already existed. So I don't really know why this was done twice...
@quietone, additionally I don't see your comment in the MR 10700 about a comment that has to be changed.
Rebased the MR onto latest 11.x branch and fixed a new PHPCS issue.
@smustgrave, could you explain better what do you mean by backwards compatibility and in which case should we trigger an error?
@smustgrave, to properly test this, we would need a comprehensive kernel test for the FileAccessControlHandler
. I feel uneasy about testing just the 'update' operation solely to ensure that a PHP notice is not triggered. Could you provide some guidance on the best way to test cases like this? Additionally, is a full test of the FileAccessControlHandler
within the scope of this issue?
Thank you!
vidorado → made their first commit to this issue’s fork.
Added a Kernel test to test the new behavior.
vidorado → changed the visibility of the branch 3251378-block-plugins-dont to hidden.
vidorado → changed the visibility of the branch 3251378-8.9.x to hidden.
vidorado → changed the visibility of the branch 11.x to hidden.
vidorado → made their first commit to this issue’s fork.
Rerolled patch from #14
All threads resolved! :)
Thanks for the quick review!
I've replied to some comments. Please, let me know if this additional comment on the issue queue was not necessary.
vidorado → made their first commit to this issue’s fork.
vidorado → changed the visibility of the branch 11.x to hidden.
Added a Unit test that I belive is enough to test the funcionality.
In the "Remaining tasks" section there were kernel test and cache invalidation test suggestions, but I think they are unnecessary.
vidorado → made their first commit to this issue’s fork.
vidorado → changed the visibility of the branch 11.x to hidden.
Fixed a couple of things and added a unit test covering the case :)
vidorado → made their first commit to this issue’s fork.
vidorado → made their first commit to this issue’s fork.
Rebased onto latest 11.x branch.
Multiple tests are failing now, but I haven't managed to fix them. RenderElementBase::preRenderGroup
and RenderElementBase::preRenderGroup
seem to do something that makes some elements don't appear on the page on functional tests.
vidorado → made their first commit to this issue’s fork.
Oops! I'm sorry, I was so focused on the test pipeline passing that I overlooked the merge conflicts :)
Thanks for the review!
That was a hard one! :)
I've just created a MR and added a Kernel test :)
vidorado → made their first commit to this issue’s fork.
Addresed :)
I’ve contributed my two cents and implemented it as a unit test, as the necessary mocking was straightforward, just like in the existing MenuActiveTrailTest
unit test.
I've tested the MenuActiveTrail::getActiveLink()
method, which should return the first <front>
route menu link when the current page is the front page and corresponds to a route without its "own" links. This scenario occurs in a default fresh Drupal installation, where the home page is the view.frontpage.page_1
route and the main navigation menu has only a "Home" link provided by the system
core module for the route <front>
.
vidorado → made their first commit to this issue’s fork.
Just figured out that the failing of:
- Drupal\Tests\package_manager\Build\PackageInstallTest
- Drupal\Tests\package_manager\Build\PackageInstallSubmoduleTest
- Drupal\Tests\package_manager\Build\PackageUpdateTest
with the error message
Some modules have database schema updates to install. You should run the database update script immediately
was caused by the incorrect placement of the post_update hook function in the link.module
file instead of the required link.post_update.php
file.
Apologies for the mistakes, @smustgrave! I’ve gotten used to avoiding modifications to type hints in existing code, as Berdir mentioned it was outside the scope of a bugfix issue. That said, I’m more than happy to embrace the 'boy scout' principle by improving types and docs! 😊
On another note, I’m adding an additional PHPCS sniff to my pre-commit script to ensure I don’t miss any more {@inheritdoc} annotations in the future. 😅
Added backwards compatibility with previously valid CSRF tokens.
Oops! sorry, I missed that! I've now updated the Issue Summary.
While working on it, I realized that previously generated CSRF-protected links will no longer work after this change, making it a breaking change.
Leaving the status as 'Needs work' until I find a solution to address this issue. Most likely, a fallback backward compatibility check can be added if the initial check fails. I think this is the least disruptive approach, although it will not be entirely clean.
- Updated Issue Summary.
- Rebased the MR over the most recent 11.x branch.
- Resolved all GitLab threads.
- Added a post_update hook and a test for it.
The MR Gitlab pipeline is failing and I believe it has to do with some unrelated problem in the 11.x branch. Not setting this issue to "Needs Review" until this is solved.
vidorado → changed the visibility of the branch 11.x to hidden.
vidorado → made their first commit to this issue’s fork.
Thanks for the review @smustgrave :)
All threads resolved!