@codebymikey

Added support for the graphicsmagick and imagemagick binaries. graphicsmagick is a newer and slightly faster fork, so it's checked for first, before falling back to imagemagick, and then the PHP extension.

The generated JPG files are slightly smaller but of similar quality.

Patch available here.

codebymikey → made their first commit to this issue’s fork.

Thanks for the quick reply! Would it be possible to create a tagged release for it so we can target that version going forward?

codebymikey → created an issue.

Thanks for taking this up, I wasn't able to replicate the issue using the UI since the constraint validation stopped it from occuring.

I just came across the entity with the unpublish_state as it was (NULL), but not exactly sure how it got to that state. Just that it couldn't get itself out of the state.

Looking at the revision history, the state was as follows:

So it's possible the state was changed using a different mechanism that didn't check for validation constraints (not sure as there are no revision log messages for it).

Either way, in the event that an invalid transition is reached, the current error handling is such that it'll always clear the target state, and after the module returns -1, it'll be resaved with the now NULL target state and result in the loop.

I think the way to provide a test case for it is to change the moderation state directly rather than through the UI, so that the -1 cases can be properly tested for.

Created settings for debug/verbose logging:

codebymikey → created an issue.

The original issue might already be addressed by 🐛 Gin vertical toolbar submenu's overlap Active as I couldn't replicate the issue on there, so updating the issue to better support for Gin (by having a dedicated CSS file) as well as improving theming support by removing most of the inline styles and JS in favour of stylesheet inheritance via CSS custom properties in the form of --environment-indicator-background and --environment-indicator-color.

codebymikey → made their first commit to this issue’s fork.

then why don't user password reset hashes and similar?

They probably do as well, but from my use case so far, user resets are rarely done, and not necessarily deterimental to the user experience enough since the user reset hash can be easily regenerated to give a different hash that won't match the questionable URI path, whereas asset hashes appears to always be the same even after a cache clear and can cause the rest of the site to be non-functional or appear broken.

One question I've had is whether we even need to base64 encode the SHA256 hashes in the first place? It's already URL safe as hexadecimals, and base64-ing it doesn't appear to make it more secure than it currently is, it just provides a slightly shorter string.

The issue is that most sites/modules rarely take translations into account, but there is a use case for it.
I think translations isn't something that should be handled on the templating level (given it's based on the whatever language is active at the time), and is precisely the reason the date_format config property is made to be translatable, and scales a bit better.

The additional code is merely to support the UI translation elements of things (which I feel is the best option for full translation support without reopening future issues), however if you're strongly against the addition of the UI elements, we can have it be a hidden translatable config that can be overridden by the site builder as necessary similar to how $config['image.settings']['allow_insecure_derivatives'] = TRUE; is used in core (the config exists, but there's no native UI to actually enable it).

The xxHash is an interesting one, I've created a quick 3v4L PHP snippet to showcase the outputs from the various hashing algorithms.

It appears xxh64 already provides a fairly short 16 character hexadecimal output as it's a 64-bit hash, so that seems like the way forward. I'll try putting in a MR for the changes in due course and see if the change breaks anything.

codebymikey → created an issue.

Work done so far in the MR provides a way to provide theme.json support in Drupal (it can be added as a theme.json file or the <theme>.gutenberg.theme.yml file.

But requires pulling in a bunch of WordPress code in in order to properly render and manipulate the blocks as WordPress does.
This provides the side effect of us essentially needing to reimplement a lightweight version of the wordpress filter system in order to get things to function as they should.

Updated the patch to address a bug which results in potentially thousands of revisions being created if the content editor happens to transition to the target moderation state before the scheduler is able to handle it.

It keeps failing and creating new revisions because:
1. It's unable to transition back to itself.
2. The target moderation state is set to NULL.

codebymikey → made their first commit to this issue’s fork.

codebymikey → created an issue.

Following this change, the Original (full) image style is no longer visible in 3.x.

codebymikey → created an issue.

Provided a MR restricting the loaded image styles.

Not sure what happened, but the changes didn't touch or should break any of the functional JS tests.

It's now passing following a rerun without any specific changes, so it's something caused by upstream or Gitlab CI as I've experienced something similar in the past.

Created a PR addressing the issue.

codebymikey → created an issue.

After revisiting #3004425-58: Date formats with abbreviated month name (M) and ordinal suffix are not translated using context → , I believe the best option is to make the day format a config that can be translated by site builders based on the language.

This removes the reliance on core, and provides support for ordinals that might show up before the number rather than after.

Introducing the config allows us to further extend the translations/theming in the future.

Support for ordinal suffixes is a bit more complicated (e.g. https://www.omniglot.com/language/dates/welsh.htm), but providing some translation is better than using English ordinal text within a different language.

And for the more complicated ones, an ordinal dot could could be used.

Available reference: https://3v4l.org/jXP5l#v8.4.4

Test failures are due to a separate issue with functional JS.

On further investigation, it's actually not as straightforward as I thought as some languages don't support ordinal suffixes, and outright skip them. Using translations using the .po file doesn't accept empty strings as far as I'm aware.

https://en.wikipedia.org/wiki/Ordinal_indicator#Ordinal_dot

The only workaround to enforce the empty string is potentially something like this in your settings.php:

$settings['locale_custom_strings_cy']['date_ordinal_suffix'] = [
  'st' => '',
  'nd' => '',
  'rd' => '',
  'th' => '',
];

The only other option is to introduce a translatable config which users can configure to choose the date format to pass into the date formatter (that way, they can choose to omit S or use a different notation as necessary)

We could try looking into using NumberFormatter for the ordinal, but then we lose the flexibility for users to override the translation to support their specific use case e.g. some clients would prefer to remove the ordinal suffix, or use a custom one instead.

Updated issue to include support for missing ordinal suffix jS.

codebymikey → created an issue.

Also worth checking if this could be consolidated into 📌 Reintroduce Views integration for locale.module Needs work since it makes integration a whole lot easier.

codebymikey → created an issue.

codebymikey → created an issue.

Patch works fine for me.

Provided patches and MR for two approaches.

1. Dropping support for 1.x.
2. Adding support for both versions.

The implementation route is down to discretion of the maintainer.

codebymikey → made their first commit to this issue’s fork.

Updated the issue fork so that vocabulary_vid arguments (Taxonomy term: Vocabulary) can be used to override the page title using {{ arguments.vid }}, since it was previously querying the taxonomy_term entity type.

Whilst testing for the #155 issue, I was unable to replicate the bug on a clean install targetting that branch.

I however did run into a separate issue where following the same instructions on a clean 11.x installation, the selected media element is not actually inserted into the page. I'm not sure if a separate media UI issue should be created for it, or addressed as part of this AJAX. But the commit for it is available here if it needs to be moved out.

I was unable to replicate the #168 issue. I've attached a copy of the latest PR, and if you're still having issues, please provide exact instructions on how to replicate on a clean 11.x installation with just the patch applied.

codebymikey → changed the visibility of the branch 2842525-ajax-attached-to-views-exposed-filter-no-variants to hidden.

Drupal appears to cache the contextual links inside the browser's session storage.

So in order to pick it up for existing content, you'll need to manually clear out the session storage so it can be refetched.

Upon upgrading to the latest 3.0.5 version, I was finally able to replicate what my original issue was.

And can be replicated by creating the attached piece of content → on a WordPress installation running Gutenberg 10.7.0 (equivalent of Drupal's v2.14) and WP 5.9, then later viewing the same content after updating to Gutenberg 16.7.0 (equivalent of Drupal's 3.0.5).

The frontend styles and HTML has changed enough that it no longer uses margins for the spacing, however, those same classes are currently missing in the current Drupal implementation and messes up the layout:

The expected layout should be something along the lines of this:

codebymikey → created an issue.

Attached a patch for 10.3.x

codebymikey → changed the visibility of the branch 2842525-ajax-attached-to-views-exposed-filter to active.

codebymikey → changed the visibility of the branch 10.3.x to hidden.

codebymikey → changed the visibility of the branch 2842525-ajax-attached-to-views-exposed-filter to hidden.

The current iteration of the patch triggered the same bug as in 🐛 media_library_opener leads to massive GET requests that break varnish etc. Active for AJAX based media libraries (or any view with pagination by creating an extremely long URL path as it serializes all the POST data as GET parameters).

It ends up picking up all the POST data and using them as the exposed input query string (which is what the pager uses).

I've updated the logic so that it only picks up POST data that have been explicitly configured as being exposed by the handler.

Attached two solutions, one which picks up all exposed inputs which start with the same id → as the exposed filter (because some bespoke filters might have compound fields which have the same prefix, so they might be harder to pick up using the default exposed filter - however this is also a bit of an edge case), and one that should work for about 95% → of all use cases.

codebymikey → made their first commit to this issue’s fork.

Removed the needs test tag, and used a more appropriate title.

As per #2, I believe this is expected behaviour, and opting out of it for content moderated content probably isn't advisable, and would cause more issues.

I'd recommend that all development efforts are placed into 🐛 New non translatable field on translatable content throws error Needs work since that addresses the actual issue at hand (stopping it from applying data from non-translatable fields when they're inaccessible).

Depending on how the module works, using this patch can also result in loss of data if the widget returns an empty value when it detects that translations are unsupported as per my smart_date patch (which is probably why Paragraphs are a problem as per @stefvanlooveren - see #3194515-4: Smart Date Recurring doesn't work in non translatable field → )

I've updated the issue summary a bit more to replicate it on a clean install with the 🐛 New non translatable field on translatable content throws error Needs work patch.

As well as attached a copy of a 10.3.2 sqlite database → to demonstate the behaviour. The relevant settings.php setting is as follows after decompressing it:

$databases['default']['default'] = array (
  'database' => '/var/www/html/web/core/default/files/.smart_date-3194515._ht.sqlite',
  'prefix' => '',
  'driver' => 'sqlite',
  'namespace' => 'Drupal\\sqlite\\Driver\\Database\\sqlite',
  'autoload' => 'core/modules/sqlite/src/Driver/Database/sqlite/',
);

And can be triggered by trying to save the translation on /ja/node/1/edit.

The patch in 🐛 New non translatable field on translatable content throws error Needs work helps but doesn't solve this issue because the "original" field value is still being modified when the RRule is initially read in the translation and causes the \Drupal\Core\Entity\Plugin\Validation\Constraint\EntityUntranslatableFieldsConstraintValidator::hasUntranslatableFieldsChanges() call to return TRUE.

Rebased for 11.x, adding support for the 📌 [ignore] Convert everything everywhere all at once Active work.

Version of the patch still using the procedural approach is available here for reference purposes.

It doesn't appear the MR has been merged in.

codebymikey → created an issue.

codebymikey → created an issue.

codebymikey → created an issue.

Attached a static copy of the proposed patch in #22 for those who need it.

codebymikey → created an issue.

The ideas from #27 are interesting enough (especially from an optimization standpoint if you don't want to integrate a reverse proxy for caching purposes).

The https://www.drupal.org/project/page_cache_query_ignore → module seems to address a subset of the functionality without the necessary granularity for controlling the query strings supported for specific routes. The ideas proposed here can be potentially incorporated in that module.

Provide a link to the "Layout Builder Expose All Field Blocks" deprecation issue

Yeah, that's completely fine, feel free to update the issue summary/CR and issue tags as necessary.

@oily, I get your point, I think it just depends whether the XSS filtering is necessary in the first place given how the rendering supposedly works and views being configured by the site builders with admin permissions rather than arbitrary users.

If XSS filtering is not necessary, then removing it solves the actual issue rather than fixing it for this specific video/iframe situation - since the real issue is that the same supposedly "dangerous" HTML elements are rendered on the page when rewrites aren't turned on.

However if the core team determines that filtering is still necessary, then we can make a follow-up issue with direction for how they propose we go about opting out of it. But I think it's worth raising those questions here before this lands.

The more I think back on this, the more I think the current approach wouldn't actually solve the long-term issue (we'll forever be needing to add new elements as necessary to fit the site's use-case).

I think we can keep the API changes to \Drupal\Component\Utility\Xss::filterAdmin() as it's helpful for modules.

But we probably also require a UI option to either:
1. Skip XSS filtering entirely on the specific field rewrite,
2. Allow the user to specify the additional HTML elements which are exempt from XSS for view fields (not sure if this should be a global setting, or on an individual field-level basis).

More thoughts on this are more than welcome.

codebymikey → created an issue.

codebymikey → changed the visibility of the branch 3295745 to hidden.

codebymikey → changed the visibility of the branch feature/3295745-multiple-connectors-5.0.4 to hidden.

The only part I'm not sure I understand is why the div wrapper is needed in the fallback render.

It was necessary because without it, the HTML output was in a different format than if it still had an RRule attached - I think from memory, it'd be just the inner content which makes styling a bit more difficult.

As a potential alternate approach, what about validating the rule object during the initial loop through the $items array?

That'a a good idea, but given the way the rest of the code worked wrt loading the SmartDateRule, I didn't want to move the logic around and potentially break something else or make it harder to review.

Relating to 🐛 Recurring date validation is wrongly triggered even if the relevant date field is not actionable on the page (e.g. with translations) Active , ran into a bug with the current implementation where it doesn't properly process the RRules for the recurring dates when they're translated.

Steps to reproduce

1. Create a recurring date with a start and end date of 04/11/2024 01:00 PM - 04/11/2024 02:00 PM.
2. Specify that it repeats weekly on Monday, and has an ends on date of 11/11/2024.
3. Attempt to save the content, it saves just fine.
4. Translate the piece of content, and attempting to save will trigger the warning. This is because the smart date RRule was no longer being processed, but it was still attempting to validate it against the end date of 11/01/2024 12:00 AM which is smaller than the end date instance value of 11/01/2024 02:00 PM.

Proposed resolution

1. We can return early and flag the element as inaccessible with $element['#access'] = FALSE;
2. Load the smart date rules and attempt validations on it even if the field is not actually used (seems like a waste of resources).
3. Update \Drupal\smart_date_recur\Entity\SmartDateRule::validateRecurring() so it does a check if the element is not accessible to the user - if it is not accessible to the user, then it skips any subsequent validations:

if (!Element::isVisibleElement($element)) {
  // Don't attempt to validate recurring if the widget element is not
  // visible on the form - which might be the case for translations.
  // @see \Drupal\content_translation\ContentTranslationHandler::entityFormSharedElements()
  return;
}

The fix for that will be addressed in that issue so as not to conflate issues.

codebymikey → changed the visibility of the branch 3483466-recurring-date-validation to hidden.

Updated the issue summary and a MR for the fix.

A simple test case might be useful, however, one thing I've noticed for a while now from other commits and MRs is that most of existing Functional tests are failing randomly in Gitlab CI, do we have an idea as to why that is?

Hi @abhishek_gupta1, the MR partially addresses the issue, but currently has some coding standards issues as well as making changes to other unaffected parts of the code. The merge request title and commit messages also aren't the most helpful for reviewing, you should try to follow the Drupal commit message format as suggested in the Credit & committing section → of the page.

I have a fix I'm working on locally that addresses the issue whilst also supporting translated content, so will probably force-push that into the issue fork instead (but since I can't change the merge request title, I might push mine into a separate branch instead). Thanks for attempting to fix the issue though.

codebymikey → created an issue.

Provided an updated data structure for the fields, needs review by maintainers/community.

codebymikey → created an issue.

codebymikey → changed the visibility of the branch skip to hidden.

Need to reconfirm whether this is still an issue in >= 10.3x since the merging of 🐛 Media Library widget display doesn't return to first page on applying filters Needs work

Provided an initial patch incorporating the necessary changes.

A separate update hook might be necessary for this to work, or have this patch applied alongside your update to 10.3.

codebymikey → created an issue.

One thing of note is that this change breaks existing view definitions of certain argument plugins like file_fid, node_nid, taxonomy, vocabulary_vid, user_uid which directly extend the NumericArgument plugin.

Their hook_views_data() argument definition might not include the entity_type property, and triggers an exception to be called when it tries to fetch the title of the node.

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "" entity type does not exist. in Drupal\Core\Entity\EntityTypeManager->getDefinition() (line 142 of core/lib/Drupal/Core/Entity/EntityTypeManager.php).
Drupal\Core\Entity\EntityTypeManager->getHandler(NULL, 'storage') (Line: 195)
Drupal\Core\Entity\EntityTypeManager->getStorage(NULL) (Line: 75)
Drupal\views\Plugin\views\argument\EntityArgument->titleQuery() (Line: 80)
Drupal\views\Plugin\views\argument\NumericArgument->title() (Line: 1048)
Drupal\views\Plugin\views\argument\ArgumentPluginBase->getTitle() (Line: 1169)
Drupal\views\ViewExecutable->_buildArguments() (Line: 1326)
Drupal\views\ViewExecutable->build(NULL) (Line: 1450)
Drupal\views\ViewExecutable->execute(NULL) (Line: 1513)
Drupal\views\ViewExecutable->render() (Line: 133)
Drupal\views\Plugin\views\display\Block->execute() (Line: 1689)
Drupal\views\ViewExecutable->executeDisplay('block_1', Array) (Line: 81)
Drupal\views\Element\View::preRenderViewElement(Array) (Line: 61)
Drupal\views\Plugin\Block\ViewsBlock->build() (Line: 171)
Drupal\block\BlockViewBuilder::preRender(Array)

I'll open a follow-up issue for this.

Thanks @danflanagan8, I think this issue addresses a different situation, which is related to the fact that the empty value gets out of sync with the actual state of the row field.

I've updated the issue summary accordingly.

Updated the issue summary

Reuploaded an updated static patch of the latest MR.

Made the change so that it only affects the views module rather than globally allow iframes in the admin.

Some further thought is probably necessary for whether this exposes any new potential security vectors.

e.g. if the site builder is using a user supplied token like a query string as a filter. But I'd assume as long as the value isn't rendered as {{ token|raw }}, it should be properly escaped or better yet, they should be using an appropriate filter in the first place like {{ token|escape('uri') }}.

codebymikey → changed the visibility of the branch 2942327-views-strips-out to hidden.

I took a quick look, but then realized the diff was so long :o

Haha, yeah, there's quite a lot going on in that MR, but it seemed like the easiest way to address and efficiently test all the improvements from like 3 separate issues 😅.

I don't mind having this merged in first - would we need to cherry-pick for 3.x as well? I haven't had a chance to use that branch yet.