Logroño (La Rioja)
Account created on 1 December 2010, about 14 years ago
#

Merge Requests

More

Recent comments

🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)

I've finally fixed the caching behavior and all tests.

Things I'm concerned about:

  • I had to remove the weak (W/) prefix from the ETag header, as it was sometimes being added, causing multiple EntityResourceTestBase 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 a max_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 in BrowserTestBase.
  • 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 additional Accept-Encoding value is added in a second Vary header instead of being merged into the existing Vary header containing Cookie and Origin values. To account for this, I modified CorsIntegrationTest to check for "Origin" across all possible Vary headers.
🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)

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?

🇪🇸Spain vidorado Logroño (La Rioja)

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!

🇪🇸Spain vidorado Logroño (La Rioja)

Sounds good. I'll work into this :)

🇪🇸Spain vidorado Logroño (La Rioja)

Umm, this is getting interesting. Some thoughts:

  1. 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.
  2. If you believe that the final value is what matters most, then storing the previous value isn’t relevant.
  3. 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.
🇪🇸Spain vidorado Logroño (La Rioja)

@smustgrave I've replied to your comments.

Thanks for the review!

🇪🇸Spain vidorado Logroño (La Rioja)

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

🇪🇸Spain vidorado Logroño (La Rioja)

Thanks! I've replied to your comment in the MR.

🇪🇸Spain vidorado Logroño (La Rioja)

@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.

🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)

@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?

🇪🇸Spain vidorado Logroño (La Rioja)

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."

🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado made their first commit to this issue’s fork.

🇪🇸Spain vidorado Logroño (La Rioja)

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. :)

🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado made their first commit to this issue’s fork.

🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado made their first commit to this issue’s fork.

🇪🇸Spain vidorado Logroño (La Rioja)

Changing version to latest 10.x as I see in the IS that this is obsolete for 11.x

🇪🇸Spain vidorado Logroño (La Rioja)

I've managed to move the _editor_get_entity_reference_revisions() function to a helper service with unit test coverage.

🇪🇸Spain vidorado Logroño (La Rioja)

Thanks for your feedback @smustgrave, I've addressed all your suggestions :)

🇪🇸Spain vidorado Logroño (La Rioja)

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!

🇪🇸Spain vidorado Logroño (La Rioja)

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?

🇪🇸Spain vidorado Logroño (La Rioja)

@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!

🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado made their first commit to this issue’s fork.

🇪🇸Spain vidorado Logroño (La Rioja)

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));

🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)

Fixed a deprecation and restoring to RTBC

🇪🇸Spain vidorado Logroño (La Rioja)

The new numbers look reasonable to me

🇪🇸Spain vidorado Logroño (La Rioja)

I could help you out if you want. Let me know here or contact me in slack :)

🇪🇸Spain vidorado Logroño (La Rioja)

All threads are now resolved. Thanks.

🇪🇸Spain vidorado Logroño (La Rioja)

Done!

I guess I can safely restore to RTBC...

🇪🇸Spain vidorado Logroño (La Rioja)

@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! :)

🇪🇸Spain vidorado Logroño (La Rioja)

Thanks for your suggestions @longwave!

🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)

Summary lines look good to me :)

🇪🇸Spain vidorado Logroño (La Rioja)

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!

🇪🇸Spain vidorado Logroño (La Rioja)

Created a MR, applied the patch from #5 and added a kernel test.

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado made their first commit to this issue’s fork.

🇪🇸Spain vidorado Logroño (La Rioja)

Trying to get feedback

🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)

Rebased the MR onto latest 11.x branch and fixed a new PHPCS issue.

🇪🇸Spain vidorado Logroño (La Rioja)

@smustgrave, could you explain better what do you mean by backwards compatibility and in which case should we trigger an error?

🇪🇸Spain vidorado Logroño (La Rioja)

@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!

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado made their first commit to this issue’s fork.

🇪🇸Spain vidorado Logroño (La Rioja)

Added a Kernel test to test the new behavior.

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado changed the visibility of the branch 3251378-block-plugins-dont to hidden.

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado changed the visibility of the branch 3251378-8.9.x to hidden.

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado changed the visibility of the branch 11.x to hidden.

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado made their first commit to this issue’s fork.

🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado changed the visibility of the branch 11.x to hidden.

🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado made their first commit to this issue’s fork.

🇪🇸Spain vidorado Logroño (La Rioja)

Fixed a couple of things and added a unit test covering the case :)

🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado made their first commit to this issue’s fork.

🇪🇸Spain vidorado Logroño (La Rioja)

Oops! I'm sorry, I was so focused on the test pipeline passing that I overlooked the merge conflicts :)

Thanks for the review!

🇪🇸Spain vidorado Logroño (La Rioja)

I've just created a MR and added a Kernel test :)

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado made their first commit to this issue’s fork.

🇪🇸Spain vidorado Logroño (La Rioja)

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>.

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado made their first commit to this issue’s fork.

🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)

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. 😅

🇪🇸Spain vidorado Logroño (La Rioja)

Added backwards compatibility with previously valid CSRF tokens.

🇪🇸Spain vidorado Logroño (La Rioja)

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.

🇪🇸Spain vidorado Logroño (La Rioja)
  • 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.

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado changed the visibility of the branch 11.x to hidden.

🇪🇸Spain vidorado Logroño (La Rioja)

vidorado made their first commit to this issue’s fork.

🇪🇸Spain vidorado Logroño (La Rioja)

Thanks for the review @smustgrave :)

All threads resolved!

Production build 0.71.5 2024