San Francisco
Account created on 29 September 2004, over 19 years ago
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Updated the summary. I think this issue makes more sense now than when originally proposed, as C.UTF-8 has been much more widely standardized and adopted than it was back then. However, should still be thoroughly tested.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@catch the part that confuses me is that asset data is both cached in the cache_data bin, and also sent to the browser in the long query string. I'd have thought one or the other would be sufficient. But, I didn't read thru all the related issues yet, I'm probably missing some basics.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Requiring zlib doesn't seem like too big of an ask. But has me wondering - I guess there was some reason why it's better to pass all the asset data in long URLs, rather than just storing it persistently and looking it up via the asset file name when needed?

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Hiding obsolete patch files

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

I added a test so this is ready for review.

Since this issue was created, a C.UTF-8 locale was officially added so should be even more common across different environments.

However, I changed the setlocale() to use good ol' C as a fallback for older systems: setlocale(LC_ALL, 'C.UTF-8', 'C');

As a result there will be some behavioral differences across environments, but it seems like a net win to enable correct UTF-8 handling on newer systems, while still supporting older systems.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Setting to needs work as the test still needs to be ported from patch to merge request.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

AFAIK this is still a valid issue as originally described. I created a merge request with the setlocale() change to see if tests pass. The test from the patch would still need to be ported, however.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

mfb β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

This should, AFAIK, now be resolved upstream in drupal core, because file_system no longer depends on logger. Thus, generally speaking, cache services no longer require a logger... and so a logger such as this module can depend on config, which depends on cache.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

I tried to reproduce the circular dependency described in #86 on the 10.3.x branch, and what I got was

Circular reference detected for service "scheduler.manager", path: "scheduler.manager"

which is apparently caused by the need to log a TypeError: πŸ› [error] TypeError: Drupal\scheduler\SchedulerManager::__construct(): Argument #6 ($eventDispatcher) must be of type Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher, Symfony\Component\EventDispatcher\EventDispatcher given in Drupal\schedule Active

Once I manually patched this bug in scheduler module, the ServiceCircularReferenceException was also resolved. But, this may be a different circular dependency than the one @SocialNicheGuru described.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco
πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco
πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco
πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Backport ready for review

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Tests are passing

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

I don't think LibreJS browser extension needs to be changed at all (except that it would be quite helpful if it accepted "MIT" as a synonym for "Expat")

For compatibility with LibreJS browser extension, what I'm now doing is rewriting "GNU-GPL-2.0-or-later" to "GPL-2.0-or-later" and "MIT" to "Expat"

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Added documentation to resolve #5

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@maenjuel if you have time to test out the merge request on this issue, it looks like I have this working.

We may need to add additional "canonical" license URLs for contrib modules that use other licenses.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

I circled back to working on LibreJS module for Drupal 10/11 (still slowly bringing it up to feature parity with the Drupal 7 version :)

Including remote in the array of JS metadata (as the license metadata currently is) would allow me to cleanup and simplify some LibreJS code.

And adding a source: attribute for each JS file would be perfect for all the minified libraries that are (somewhat surprisingly :) still part of core (jquery, jquery.ui, etc.)

So this issue would still be super useful for me! If there is anyone looking for a project this summer or what have you (and it's on my list if I find some time)

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Here's a pretty hacky attempt to maintain a list of aggregated JS that we can render on the jslicense page.

Ideally there is a much more elegant way to do this that someone could suggest :)

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Looked thru the related code briefly, and unlike in Drupal 7, I didn't yet find a super simple way to generate a list of aggregated JS files.

So, we might have to jump thru some hoops to build and maintain this list? I do have some ideas for how to do that, though.

In Drupal 7 this could be gathered simply via variable_get('drupal_js_cache_files');

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

I still haven't tested the 2.x branch with LibreJS browser extension, but I suspect there are three issues:

  • LibreJS extension might check the URL of the license - it might need to match the "canonical" URL of the license to be recognized?
  • We need to provide a source URL for every script. The source URL could just link to the script itself.
  • We need to list all aggregated javascript files on the jslicense page, otherwise they will not be recognized.
πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco
πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

I filed πŸ› Use standardized SPDX license identifiers Active to get core to use the standard SPDX license identifier, i.e. remove the "GNU-" from "GNU-GPL-2.0-or-later"

Although it sounded like LibreJS extension might also need to be patched to support the standard identifiers...

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

mfb β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

mfb β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Ok I added a link on the project page

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco
πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco
πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco
πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco
πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Just read thru this issue and thought I should correct my comment in #45 - I was thrown off by the "Implement strict typing" title which made me think of strict_types, but this issue seems to be more about just adding type declarations, not strict_types. So in that context, adding a string type declaration with \Stringable objects being passed in should be fine.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

mfb β†’ changed the visibility of the branch 3315004-add-tracepropagationtargets-config to hidden.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

mfb β†’ changed the visibility of the branch 5.x to hidden.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@mstrelan: Seems not worth it for the reasons I said in #6 but, if you want to discuss you could open a new issue? It seems only tangentially related to this issue, which was just to make the level map available, not to change any public APIs.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@mstrelan I think you're suggesting an API change, if the RfcLogLevels would be objects rather than simple constants? That doesn't seem worth the developer friction to do.

Ok, I guess you could have an enum that also has the original constants pointing to the enum values, and there would be no API change, but then, what's the point?

Note also that Drupal\Core\Logger\RfcLogLevel is basically drupal's (moar RFC-compliant) version of Psr\Log\LogLevel, so it seems reasonable for them to have the same architecture.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

This worked in my quick testing, but please re-open if there are any problems

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

The recommended php config for production has zend.assertions at -1, and I think this will be pretty commonly installed. And so you have that warning message logged for every request. My 2 cents is that setting zend.assertions is something that should be documented for developers to do, rather than happening out-of-the-box

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Great! I believe currently we have a pending feature request / patch for Drupal 7 which I've not yet had time to review. So that suggests I don't have capacity to help maintain a backdrop port either (at the moment)

If you provide a link to the port, I could link to it from the drupal.org project page

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Postponed until 6.x branch is open

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

mfb β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

...but these extra events weren't actually sent - at least in my testing - until I also updated the JS SDK? So I guess we also need to update that (unless I just had bad luck trying to test this feature)

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

In my testing, enabling this results in a separate ui.interaction.click payload being sent to Sentry; i.e. this is analogous to other JS payload types that we do have settings for, namely, auto session tracking and client reports. So I'd say it's preferable to add a config flag, so if someone doesn't want to be so "chatty" w/ Sentry they can keep it disabled.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

If there is any downside to enabling INP, such as breaking older versions of self-hosted Sentry server, then we might need to hide this behind a config flag. But, I didn't check for that yet so, keeping this simple for now πŸ™ˆ

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

mfb β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

eesh, seems not-ideal to wait until most-everything is destroyed to commit transactions, i.e. difficult to respond to various transaction commit failure modes.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

I was maintaining only the Drupal 7 branch for security issues, so I abstain from discussion about other branches (or, other issues on the Drupal 7 branch for that matter).

(If suppose if someone wanted to hire me to work on this project perhaps I could, but otherwise just don't have time sadly)

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Ideally the phpdoc could stay in sync with the code?

e.g.

   * @param array $ids
   *   An array of entity IDs, or NULL to load all entities.
   *
   * @return static[]
   *   An array of entity objects indexed by their IDs.
   */
  public static function loadMultiple(array $ids = NULL);

becomes

   * @param ?array $ids
   *   An array of entity IDs, or NULL to load all entities.
   *
   * @return static[]
   *   An array of entity objects indexed by their IDs.
   */
  public static function loadMultiple(?array $ids = NULL);
πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Looks like when compressed in a git packfile, this baseline compresses pretty well, down to 851KB of space

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Answering part of my own question: If you discard an event in Sentry, you still have to send it there, which could cost 150ms time (which is a lot!), unless Sentry or Sentry Relay is running locally.

So discarding in Sentry is a workaround, but not a great workaround.

Better to use the before_send hook or add this feature.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

mfb β†’ changed the visibility of the branch 3409587-rss-feeds-invalid to active.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

mfb β†’ changed the visibility of the branch 3409587-rss-feeds-invalid to active.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@OMD's followup bug report is at: πŸ› Channel description of RSS feeds is double-escaped Needs review

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Added unit test coverage for & in channel description element.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@cilefen Yes, that's what I found when trying to reproduce the issue. If we confirm that the issue is basically the opposite of the title then I will update it :)

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@OMD can you test my attempted fix in MR 6842? If it resolves the warning then we can update issue summary, add a test and reroll as a merge request on 11.x branch.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

This does seem to be pretty heavily related to πŸ› [10.2 regression] RSS feeds invalid due to   Fixed after all, although it's a separate bug in the same code.

It appears that \Drupal\Core\EventSubscriber\RssResponseRelativeUrlFilter::transformRootRelativeUrlsToAbsolute() is operating on channel description elements, but it should not, as these are considered to be human-readable plain text. It should only be operating on item description elements, which are considered to be markup.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

What I found when trying to reproduce this issue is that Views outputs <channel><description>Training &amp;amp; Events</description></channel> as the feed description.

However, it is recommended to output <channel><description>Training &amp; Events</description></channel>

My interpretation of what the feed validator is saying is that it's recommended that a feed description be semantically considered to be plain text, and thus a user-entered feed description should be encoded once to be rendered in the feed description XML element: <channel><description>Training &amp; Events</description></channel>

An item description, on the other hand, could be semantically considered to be markup, thus a user-entered item description would be encoded once to render valid markup from the entered text, and a second time to be rendered in the item description XML element: <item><description>Training &amp;amp; Events</description></item>

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@OMD, I believe I was able to reproduce what you're talking about. What I see at https://validator.w3.org/feed/#validate_by_input is:

This feed is valid, but interoperability with the widest range of feed readers could be improved by implementing the following recommendations.

line 6, column 42: description should not contain HTML: &amp;

<description>Training &amp;amp; Events</description>

In other words, the feed is valid, but there is a recommendation that Views should simply output <description>Training &amp; Events</description> rather than double-encoding it as <description>Training &amp;amp; Events</description>

You'll have to discuss that in another (new or existing) issue - this issue is about feeds being actually invalid and unparseable due to &nbsp;

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@OMD yes this was released in 10.2.3 - if you're still seeing an issue you should provide steps to reproduce so folks can look into it.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@catch: I have manually tested that background=1&mute=0 can be added to Vimeo URLs via hook_oembed_resource_url_alter() - in fact, I tested this in the test hook, media_test_oembed_oembed_resource_url_alter(), although I didn't (yet) push a commit with that change (see comment #16).

So the premise in the original issue was incorrect. Anyone who wants to add background=1&mute=0 to their Vimeo embeds needs to implement hook_oembed_resource_url_alter(), parse $parsed_url['query']['url'] to get the user-provided query parameters, and if background or mute are present, add them to $parsed_url['query']. If this is an expected use case for Vimeo, then hypothetically this logic could be implemented in core itself, or else in a contrib or custom module, as I mentioned in the issue summary.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Seems like this issue is missing from the 10.2.3 release notes, however. Maybe someone could add it in retroactively...

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@HeikkiY I'm pretty sure this might have only been released in 10.2.3 because 10.2.2 was a security release

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@Liam Morland currently drupal and PHP allow \Stringable objects, strings, integers, floats and Booleans to be passed to these functions. NULL is also allowed, but triggers a PHP deprecated notice. So to avoid a breaking change, you'd have to allow that full set of accepted types.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@alexpott I'd say it should still be mentioned in the change record that logging is removed from those other methods, not just chmod. But you removed my mention of those. This is relevant if the calling code caught the exception and assumed an error was already logged by the file_system service.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

I updated the change record to more completely describe the impacts of logging being removed. Now I realize it needs a bit more work as I see Alexpott added an additional new change record, so they are partially redundant.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

mfb β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Hiding the patch file as it could confuse reviewers

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

mfb β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

In that case then I guess this doesn't need followup, we can add return types

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@smustgrave Sure

By the way, where the coding standards say

Parameter and return typehints should be included for all new functions and methods, including new child implementations of methods for existing classes and interfaces.

does this mean LazyConfigFactory should go ahead and add type declarations where its parent ConfigFactoryInterface only has docblock annotations? It's not 100% clear to me what that sentence is saying.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@smustgrave should we create a followup issue to add type declarations to ConfigFactoryInterface and ConfigFactory, at which time they could be added to LazyConfigFactory too?

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@nikhileshpaul this issue was filed re: Drupal 7; there is no "accepted answer" in this issue for later versions of Drupal.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

The method simply moved from one class to the other for consolidation purposes, but it seems useful to leave it in the main class both for backwards compatibility and to make clear that it can be used as an "API" method.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@SpaceGoat1701 how about if we move it back to the main FileHash class for backwards-compatibility reasons, rather than creating a new standalone service?

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

While we're changing a property on Drupal\syslog\Logger\SysLog, perhaps we should also "upgrade" this class to use constructor property promotion? Just pushed a commit doing so.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

I've only tested the drupal 7 version with LibreJS browser extension, so contributions are welcome + encouraged.

Sounds like possibly some improvements could be contributed to LibreJS as well.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

I'm not super familiar with media module, so not sure this is the best place for it, but here is a hopefully a working test

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@quietone If desired, we could test that multiple query parameters can be added via hook_oembed_resource_url_alter(). Currently, only altered=1 is tested, but we could change the test to set background=1 and mute=0 (spoiler it works fine)

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

Yes πŸ“Œ Sort out @backtrace_string logging by Syslog module Active is related; I'd agree that syslog module should somehow improve handling of multi-line log messages, especially since core can log those multi-line backtraces

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@opsdemon \o/ great that we understand your situation.

RssResponseRelativeUrlFilter dates from 2016, so I'm thinking perhaps a change record is needed, if there are any other sites out there that were mistakenly doing an extra round of HTML encoding on their RSS item description, and relying on RssResponseRelativeUrlFilter to transform it back. They can now remove that extra round, although luckily if they don't, there shouldn't be a security issue, just ugly HTML showing up unexpectedly.

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@opsdemon Yes, you (or someone) needs to reproduce your issue on a fresh install. Theoretically one can upload their config yml/database dump/custom code/etc. here, but better to reproduce on a fresh install to get the minimal steps to reproduce, rather than maximum steps to reproduce :)

πŸ‡ΊπŸ‡ΈUnited States mfb San Francisco

@opsdemon If I use Views module to create an intentionally double-encoded RSS feed, then without this patch, the Views preview shows the double-encoded feed, but rss.xml may be "fixed" by RssResponseRelativeUrlFilter (i.e. yes, it can mask the double-encoding by unencoding the ampersands, although the feed may also be invalid depending what it unencoded). With the patch, I see the double-encoded feed in both the Views preview and rss.xml. Is this what you're seeing? If not, good to know, but not much I can do about it without steps to reproduce.

Production build https://api.contrib.social 0.62.1