🇧🇪 (Ghent)
Account created on 24 October 2008, over 15 years ago
#

Merge Requests

Recent comments

🇧🇪Belgium mr.baileys 🇧🇪 (Ghent)

Agreed with @Anybody that current behavior when saving a translation of a media entity still is buggy. However, since in this IS there is no mention of translations, I think this issue should be closed as a duplicate of 🐛 Mapped media fields are overridden with metadata on translation save Needs work , which addressed fields being overwritten when saving a translation of a media entity.

🇧🇪Belgium mr.baileys 🇧🇪 (Ghent)

I addressed the issues raised. I don't have a lot of experience with working on drupal.org issues via merge requests, so please do let me know if made a mistake.

I do think the previous point of feedback about using the correct translation of empty is valid.

I don't think this is the case: the callback is invoked when a user clicks on the "star"-icon to add a shortcut, so it only seems to make sense to add the shortcut in the active interface language.

I changed the translation source to "(Empty)" (which is already present in the core translation set.

🇧🇪Belgium mr.baileys 🇧🇪 (Ghent)

Leaving to needs review for other opinions, but the patch in #24 only addresses shortcuts created after applying the patch. Since shortcuts without titles are still stored in the database, the deprecation notices will still appear on the shortcut management page. I think a complete solution either requires an update hook, or hiding the issue (as was done in the initial patches in this thread).

Using '(empty)' is IMO an elegant solution, but should we account for languages other than English?

🇧🇪Belgium mr.baileys 🇧🇪 (Ghent)

I agree with @ibonelli in #16. The issue occurs when displaying or manipulating shortcuts that do not have their title set. This should not be possible, since 'title' is a required field on the Shortcut entity. However it is currently possible to create shortcut entities with NULL as title. The proposed solution hides the issue but does not tackle the root cause (there should not be any shortcuts with NULL as title.).

The easiest way to reproduce the bug is using the steps outlined by @loze in #10 (creating a page display on a view without a title, and then clicking the shortcut quick add link on that page), but even without Views the issue can occur on any page without a title. It does seem limited to Shortcuts added via the "quick add"-link, as the "formal" interface for adding shortcuts has the title field as a required field.

Test-only patch attached that triggers the deprecation notice.

I see a couple of options to resolve this issue:

  1. Hide the "quick add link" on pages without a title. It does not make sense to add these pages as shortcuts, since without a title the link(s) don't seem to make sense to end users. This would be confusing to end-users though (why can't I add a shortcut to this page?)
  2. Show the "quick add link" on those pages, but instead redirect it to the shortcut add form so users can provide a title manually.
  3. Provide a default when adding a shortcut to pages without a title, for example a blank string or "(empty)". Write an update hook to replace the NULLs with the chosen default (easy if we go with a blank string, might be trickier when we choose a translatable string).

To fully address the issue, we will also need an update hook.

Setting to NR for the testbot.

🇧🇪Belgium mr.baileys 🇧🇪 (Ghent)

Test failure was unrelated to this change, and tests are back to green, so reverting to RTBC.

🇧🇪Belgium mr.baileys 🇧🇪 (Ghent)

Tried to rebase but ran into too many issues, so I cherry-picked the commits and created a new MR. I have updated the CorsIntegrationTest: the original test in this issue was written against asm89/stack-cors 1.x, while 11.x uses 2.x. The latter no longer returns http status code 403 for disallowed origins (see #3128982: Upgrade asm89/stack-cors to ^2.0 to fix cacheability ).

🇧🇪Belgium mr.baileys 🇧🇪 (Ghent)

Adding an additional index on the hostname-column does make sense. I ran a test with 100k records in the {honeypot_user}-table: the query executed in \Drupal\honeypot\HoneypotService::getTimeLimit() becomes 35x faster (from 0.073 seconds to 0.002 seconds) for anonymous users.

In our case, the slave replication showed "The slave is applying a ROW event on behalf of a DELETE statement on table honeypot_user and is currently taking a considerable amount of time (61 seconds)."

The only condition used when deleting rows in Honeypot is "timestamp < _expire_limit_", so adding an index on the hostname column will not fix that issue (maintaining an additional index might even make it worse.)

🇧🇪Belgium mr.baileys 🇧🇪 (Ghent)

Original issue contains too little information to go on, and there has been no activity in this issue in over a year with no indication that this is caused by a bug in Drupal core, so closing.

🇧🇪Belgium mr.baileys 🇧🇪 (Ghent)

There is not enough information in the original issue to determine what the cause of the 403 is, but this is a support request rather than a bug. Closing since this is a support request, there has not been any activity in over a year and there is no indication pointing to a bug in core.

🇧🇪Belgium mr.baileys 🇧🇪 (Ghent)
  1. +++ b/core/assets/scaffold/files/default.services.yml
    @@ -160,15 +160,33 @@ parameters:
    +    # Setting exposedHeaders: ['*'] will result in a Access-Control-Expose-Headers:
    +    # * response header. See
    

    This line should wrap at 80 characters. This will also fix the fact that header name and value are currently split over 2 lines.

  2. +++ b/core/assets/scaffold/files/default.services.yml
    @@ -160,15 +160,33 @@ parameters:
    +    # Setting Access-Control-Max-Age header value to '0' or false will omit this from the response.
    +    # However, setting it to '-1' will explicitly disable caching. For example, 600 to
    

    Line wrapping should be fixed.

It might make sense to merge 🐛 Improve documentation for cors configuration. Needs work and this issue, since that issue is about adding and documenting the new allowedOriginsPatterns-option to cors.config in default.services.yml.

Production build 0.69.0 2024