@codebymikey

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.

In the spirit of making the image size support more complete, I've added a commit to the merge request which incorporates changes from ✨ Allow API to restrict which image styles are available to users Active and 📌 Followup: SVG's should not use image styles Active

Work on this is being handled in 📌 Clean up gutenberg.settings config Active , and relevant credit is being attributed to that issue instead

I tested the merge request and I believe it works differently to what was proposed.

My proposition was to:

1. Remove the update hook, and instead leave the config empty until the site builder specifically chooses to filter to specific image styles (keeping backwards compatibility)
2. Stop auto-filling the entire list of image styles by default (as part of this, we should also implement and fix the "all" functionality)

I'd ideally make most of those changes in this issue, but due to the change in data schema and potential merge conflicts, most of those have been implemented within the 📌 Clean up gutenberg.settings config Active merge request, and is currently awaiting review/testing.

Are you able to test that issue when you get the chance? And hopefully once others test and agree with the changes, merging that in should help improve the general DX.

codebymikey → changed the visibility of the branch 3415218-clean-up-gutenberg.settings to hidden.

codebymikey → changed the visibility of the branch 3415218-clean-up-gutenberg.settings-2.8 to hidden.

codebymikey → changed the visibility of the branch 3415218-clean-up-gutenberg.settings-2.x to hidden.

codebymikey → changed the visibility of the branch 8.x-2.x to hidden.

Also ran into this issue whilst updating to the latest 2.14, and just thought I'd put in my two cents regarding the current situation regarding the ✨ Limit available image styles for image related blocks Fixed feature.

As stated in #7, I believe new functionality like this should either provide an upgrade path or maintain backwards compatibility for site builders to use.

However the current suggested upgrade path of setting the available image styles to a fixed set of values is a regression in behaviour imo and limits flexibility since if the site has multiple node bundles, and introduces new image styles which they should all have access to, the site builder would need to manually resave all those bundles to provide support for it.

I believe the best approach to address this is to maintain backwards compatibility by keeping the default behaviour of allowing all image styles unless the site builder specifically wants to restrict it to specific ones.

Ideally the work done in ✨ Allow API to restrict which image styles are available to users Active would allow site builders/developers to restrict the image styles in a more dynamic fashion (e.g. custom permission checks via alter hooks), but that's currently only working for image uploads and will need to be adapted to support the drupalSettings['gutenberg']['image-sizes'] value too.

As part of my config normalization work in 📌 Clean up gutenberg.settings config Active , I've addressed the default value issue, provided a config schema and maintained backwards compatibility without requiring a database upgrade. So feel free to test the patch on that issue, or provide feedback if the proposed solution isn't viable.

I like the tests, so we should definitely keep it in once a decision has been made regarding backwards compatibility.

Reopened to address some config normalization issues related to the ✨ Limit available image styles for image related blocks Fixed feature which was added, but doesn't currently have a valid schema.

codebymikey → changed the visibility of the branch 3313569-fix-load-items-warning-v2 to active.

codebymikey → changed the visibility of the branch 3313569-fix-load-items-warning-v2 to hidden.

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

poker10 → credited codebymikey → .

Hi Thor,

I also couldn't replicate the issue in the latest version, and on doing some further investigation, a fix for the missing styles was added in this commit.

I think my tests at the time of writing were based off 3.0.0-beta5 which didn't have the fix, and was only added in 3.0.0-beta6.

The LayoutProcessor::isSupported() review part was added for a reason though, I haven't worked on the 3.x branch in a while, but I'll be sure to update the issue summary accordingly if I run into any specific issues when I attempt an upgrade.

codebymikey → created an issue.

codebymikey → created an issue.

Rerolled the #24 patch as a issue fork branch for 8.x-1.x, with some minor improvements (show the revision ID as a translatable text, as well as an utility revision-was-default class to dictate if the highlighted revision was a previous revision, rather than the default current revision.)

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

Updated the PR and requesting further review of the changes.

Would ideally be nice if a test was also introduced as part of this change to avoid unintentionally breaking it.

I don't have time to write a functional test for it at the moment, but if someone else wants to have a go at it, you're more than free to!

dan612 → credited codebymikey → .

Updated the MR incorporating feedback from Comment #7.

It now uses more specific labelling to distinguish what the banner is for.

And in terms of convention as to how to handle the banner. We have examples of using just the aria-label in the GOV.UK design system and this blog post.

They both do make use of region roles rather than alertdialog, so it's possible that also needs to be revisited according to best practices.

I've also attached a static patch that applies cleanly against the 1.24 version of the module, incorporating the 🐛 Declined cookies with top popup pull content out of the page Fixed patch.

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

codebymikey → created an issue.

codebymikey → created an issue.

codebymikey → created an issue.

codebymikey → changed the visibility of the branch 3476626-normalize-multi-line-configs to active.

codebymikey → changed the visibility of the branch 3476626-normalize-multi-line-configs to hidden.

codebymikey → created an issue.

Update the code a bit to meet best practices. There's still some other improvements to be made such as references to "alt tag", should probably be "alt text", but that should be handled in a separate issue with an update hook after this issue is resolved.

codebymikey → changed the visibility of the branch 3476370-use-configured-image-style to hidden.

codebymikey → created an issue.

codebymikey → created an issue.

codebymikey → created an issue.

codebymikey → changed the visibility of the branch 3014694-avoid-duplicate-entry to hidden.

It's a bit different mainly due to what they each tackle.

This addresses a potential optimization of the ExposedFormPluginBase::renderExposedForm() logic - stopping it from unnecessary building the form and its IDs when the view it's attached to doesn't actually make use of it, whereas the other issue is related to addressing when multiple legitimate variants of the exposed form are placed on the same page.

With this issue, only one instance of field edit-field-item--2 is rendered on the page (which should ideally be edit-field-item if properly addressed), whereas with that other issue, there can potentially be edit-field-item--2, edit-field-item--3 since they're legitimate duplicates of the same field, but the form they're attached to is the only thing that isn't unique.

From the failing tests, it appears the building of the form is at least somewhat necessary in order to appropriately apply the exposed form filter settings against the main view.

Some further thought might is necessary in terms of avoiding the need to build the same form multiple times for the same view.

codebymikey → created an issue.

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

It still says the to_divide field type is Number on the webform list, which would mean it's either not saved or there's something else going on with your instance.

Here's an example webform config based off the example one in the module that you can use for your test:

loan_amount:
  '#type': number
  '#title': 'Loan Amount'
  '#required': true
interest_rate:
  '#type': number
  '#title': 'Interest Rate'
  '#required': true
loan_term:
  '#type': radios
  '#title': 'Loan Term'
  '#options':
    1: '1 year'
    2: '2 years'
    3: '3 years'
    4: '4 years'
    5: '5 years'
  '#required': true
result:
  '#type': form_calculation_element
  '#title': result
  '#evaluation_decimals': 2
  '#evaluation_fields': '((:loan_amount / :loan_term) + ((:interest_rate / 100) * :loan_amount)) * :loan_term'

I think it's also worth documenting in the form or the README the fact that making use of the "Show only used items" option will load the entire result set, which is a bit inefficient and can potentially lead to performance issues depending on if the site has thousands of results or not.

I think a better approach might be to alter and build up a custom query that returns only the used terms. It might require more configuration on the user's end to facilitate this, but the performance gain will be very noticeable.

The alternative is to use Facets instead if you have thousands of results, but again, these disclaimers are probably worth documenting somewhere.

codebymikey → changed the visibility of the branch 3360478-errors-when-used to hidden.