In testing I found that the pretty path redirect response to the views exposed form submission was coming back with "no-cache" in the cache control header. Adding cacheability metadata to the pretty path redirect response seems to have solved the issue. This should improve performance on previously-requested facet filter combinations.
Attached is an immutable patch against 2.0.x representing the current state of MR 20.
Branch 3516724-views-boolean-filter-null-bug under this issue's drupal-3516724 fork now includes a fix for this bug.
Still needs test coverage, so MR not yet opened.
The fix was to anticipate and handle the possibility of NULL/empty field values when setting up the WHERE clause in the query. Relying on a condition like [field-value] <> 1 is not enough, because that check will evaluate to FALSE if [field-value] is NULL in database backends like MySQL (see ref). Expanding the condition to ([field-value] <> 1 OR [field-value] IS NULL) resolves the issue by handling this gotcha with NULL in comparisons.
The attached patch represents the 3516724-views-boolean-filter-null-bug branch's current diff against 11.x.
Proposed resolution in the issue description updated to align with the fix that is proposed here.
chrisolof → created an issue.
Thanks edsoncarlos! Looks like we missed that over in 🐛 Deprecated FormElement Active . Issue filed: 🐛 Bump up minimum Drupal version to 10.3 Active
chrisolof → created an issue.
chrisolof → created an issue.
Wow. That is a problem. Thank you for catching this. I am re-categorizing this issue as a bug report.
I lean toward adding this new check in Drupal\phone_number\PhoneNumberUtil::testPhoneNumber() because that is a centralized checker used by both Drupal\phone_number\Plugin\Validation\Constraint\PhoneNumberValidator::validate() (line 69, $phone_number = $item->getPhoneNumber(TRUE);) and Drupal\phone_number\Element\PhoneNumber::phoneNumberValidate() (line 244).
That would ensure this new check is called in (I think) all situations (render element usage, regular field usage, programmatic entity field usage, etc.) and probably avoid any duplicated error messages.
And to ensure this bug stays fixed, I think we should add a test confirming the fix. It could be as simple as submitting India + 999 into a phone number field, confirming that the submission is not accepted, and confirming the expected form validation error is displayed.
Thanks everyone! MR 21 has been merged into 2.0.x and tests are now passing in a Drupal 11 environment.
chrisolof → created an issue.
chrisolof → made their first commit to this issue’s fork.
Thanks arwillame. I think this plugin would be more at home, and likely better maintained, in the GraphQL Compose module's GraphQLComposeFieldType plugins folder. I see a bunch of other plugins just like this one, covering all sorts of contrib field types, in there already:
https://git.drupalcode.org/project/graphql_compose/-/tree/2.4.x/src/Plug...
If the GraphQL Compose maintainers are not open to this new plugin going there then I would encourage you to post this up as its own module where you can easily keep it up-to-date and running right.
If you do manage to get this into GraphQL Compose or your own contrib module let me know and I'll link us up on this module's project page so people can easily discover the connection.
Thanks bogdan.racz. These changes have been merged into our 2.0.x branch.
chrisolof → made their first commit to this issue’s fork.
Our phone input is now a tel instead of a textfield (change merged into 2.0.x). Thanks everyone!
Looking at our existing validation it certainly seems like adding a call out to \libphonenumber\PhoneNumberUtil::isValidNumber() would duplicate existing, earlier checks that validate the entered number against the chosen country. I must be missing something. Could you provide more detail on how to reproduce the issue that this addition resolves?
I totally support this idea but I agree with tim-diels that we'd want to build against a stable release of the flags module, which does not exist yet.
Since we now have two options for country selection (Two-letter ISO country code and Flag) there is some potential for making the Flags module a suggested dependency that opens up the "Flag" country selection option if present.
Thank you! This change has been merged into the 2.0.x branch.
Attached is a patch against the current 4.x branch that brings in support for compound fields of type managed_file and phone_number.
I look at this patch as a temporary fix, but not what we'd want to merge into this module. I still think the constraint route outlined above in #7 is the way to go as it would bring in support for nearly all compound fields and potentially reduce this module's codebase at the same time.
MR 12 fixes the TypeError I was running into when copying layout builder layouts with inline content blocks. Additionally, the changes in MR 12 should allow for layout cloning on sites with or without the Block Content module (and with or without inline Block Content blocks in their source layouts).
This is ready for review.
chrisolof → created an issue.
chrisolof → created an issue.
chrisolof → created an issue.
Confirming MR 262 is working great in my tests.
+1 on the number field idea. It appears these hard-coded options are just carried over from the 2.x config form. Probably it makes for a nice clear "No limit" option chosen by default instead of an empty box. Maybe we'd need to modify this text:
*NOTE: setting to "No limit" sets the hard_limit value to "0", which depending on the search backend could lead to limited results. Try setting to the maximum value if you are experiencing this issue
To something like:
Leave empty for no limit.
Note: Some search backends will still apply a default limit when no hard limit is set. If this affects you, set this to an appropriately-high value or a value that indicates "no-limit" to your search backend.
It appears this new setting still needs to be added into the module's config schema (config/schema/facets.views.schema.yml).
Does this need test coverage?
chrisolof → created an issue.
Basic test coverage added in MR 15. Verifies the whole BEF filter wrapper is hidden when the exposed filter is configured not to show in a configurable views filter block. This is now ready for review.
Re-titled the issue as it seems we're all pretty specifically after BEF collapsible details element support in here.
MR 15 adds this support, taking a slightly different approach at the code level, hopefully making it easier to add support for additional filter wrappers in the future (should more requests like this come in) without repeating code.
Test coverage not added yet, so changing status back to needs-work.
chrisolof → made their first commit to this issue’s fork.
chrisolof → changed the visibility of the branch 3509467-add-option-to to hidden.
chrisolof → created an issue.
Update: I have just achieved pretty paths atop facets exposed filters over in issue ✨ Support facets 3.x facets exposed filters Active under the Facets Pretty Paths module.
Test coverage and small improvements added (some identified while adding the test coverage). This is now ready for review.
Attached is an immutable patch against 2.0.x representing the current state of the MR 20.
chrisolof → created an issue.
chrisolof → created an issue.
Support added in the 3505618-support-facets-exposed-filters branch. Tested against a variety of facet filters with a variety of employed coders, single and multi-value selections. Seems to be working well at this point.
Pretty paths coder config is at the filter-level (it shows in the exposed filter options form). It is possible to combine pretty and regular query string facet filters (eg. "/view/category/foo-258?tag=33).
Filter order in the URL is currently just following the filter order already established in the view.
Still needs test coverage and likely some additional refinement - both hopefully coming in this week.
chrisolof → created an issue.
chrisolof → created an issue.
chrisolof → created an issue.
chrisolof → created an issue.
chrisolof → created an issue.
I also ran into this issue and the provided patch appears to fix it. Thanks khiminrm!
Thanks Liam. Test coverage added, covering the clean-up of webform_scheduled_email records with missing a plugin or a missing submission, both in schedule and send state (to ensure cleanup is working in cronSchedule() and cronSend()).
While I was in there I found some disabled test code that seemed disabled due to this orphaned record issue, so I moved, updated, and enabled that check also.
MR 595 removes orphaned webform_scheduled_email records when encountered in cron methods instead of trying to proceed with them and causing PluginNotFoundExceptions. This is ready for review.
Interestingly it seems there is some skipping and cleanup logic already in this submodule.
The solution cronSchedule seems to take currently (when the referenced webform submission is missing) is to delete the orphaned record from webform_scheduled_email and move on. Cleaning up the orphaned (and unusable) record upon discovery seems better than logging and requiring a site admin to manually clean up the table someday. As such, I'm updating the proposed resolution.
chrisolof → created an issue.
chrisolof → created an issue.
chrisolof → created an issue.
chrisolof → created an issue.
The attached patch represents the state of MR 9 at the time of this comment.
MR 9 opened. Builds on MR 3.
- Uses the module's existing 1.x plugin definition API
- Condenses the code a bit
- Expands support to entity reference revisions fields
Needs review and testing. Using this along with MR 8 (from ✨ Add ability to clone a layout builder layout field Active ) allows me to deeply clone layout builder layouts, including sub-entities referenced from layout builder content blocks inside of layout components. Pretty neat.
Like in MR 8, it's possible we could use this module's clone process to clone these referenced entities when configured, falling back to the Drupal core-provided clone. That was not necessary for my project so I haven't gone that far here, but it certainly seems like a nice-to-have for projects that might need to configure how these referenced entities are cloned.
I think the change in src/Plugin/FieldProcessorPluginBase.php is an unnecessary, breaking change that should be proposed in a separate issue. A change like that would normally go out in the next major version of a module so as not to break existing plugin implementations written against the 1.x plugin API.
Merge request created. Working so far across a variety of complex layouts in my initial tests.
- New "Copy layout" field processor plugin, only offered for layout builder layout type fields.
- Clones each section inside of the layout
- Clones each component inside of each section, giving each component its own UUID. UUIDs are not intended to be shared.
- If component is a layout builder-provided content block, the content block is also cloned (at the revision specified by the component). These blocks are not intended to be shared across layouts. Cloning these blocks prevents cross-talk issues between original and cloned layouts. Block clone is done content_entity_clone-style if clone settings are enabled for the block type, falling back to a core entity clone.
- Copy values plugin adjusted to not show up for layout builder layout type fields, as it is inappropriate for that field type.
Needs review and additional testing.
Attached patch represents the current state of MR8 at the time of this comment.
chrisolof → created an issue.
chrisolof → created an issue.
Thanks DrupalHunk! The changes in 057961e40decefada8c9b92066e5a5ab66087511 (from MR5) appear to resolve this issue.
MR5 also currently introduces a .gitlab-ci.yml file that seems to be a start at running automated testing / linting / building (of which we have none yet).
I feel this isn't the right issue to bring in a .gitlab-ci.yml file, unless it also brings in a test confirming the fix here. I've opened ✨ Automate manual tests Active , which I think would be a good spot to bring in a .gitlab-ci.yml file.
chrisolof → created an issue.
chrisolof → created an issue.
The attached patch against 2.0.x:
- adds a sort into the mix so that entry order of inclusions and exclusions is no longer important
- adds documentation of how to exclude under the textareas where exclusions can be entered
- updates some code comments to make it clear that we're working with exclusions as well as wildcards
Tests are still needed here.
Hey guys - I believe we've got this in the latest beta by way of the fix to 🐛 Not properly working with Google maps provider Fixed .
See: https://git.drupalcode.org/project/address_autocomplete/-/blob/1.0.0-bet...
Due to the decoupled nature of this functionality this provider bypasses the stock Drupal autocomplete widget in favor of the one provided by the Google Maps API. This is both for ease of use purposes, and also because attempting to implement this API using the Drupal autocomplete widget results in large numbers of expensive (as in money) queries in order to function.
The current implementation utilizes autocomplete session tokens to avoid this, and I believe is then able to lean on the regular Drupal autocomplete functionality.
Unless I'm missing something here, I believe we can close this issue as a duplicate.
From what I can tell this bug is now fixed in the latest version of this module. Code similar to what's happening in #13 is now in place, which apparently helped fix ✨ Hide output if empty option Fixed . I don't have as many usages of EVAs in LB layouts anymore, but those I do have seem to be working with the latest version of this module (unpatched).
Can others still reproduce this issue with the latest version of this module?
What specifically does the error message say? Also - be sure you've allowed (toll-free) 1-800 numbers in your field instance settings and be certain the 1-800 number you're testing with is actually a Canadian 1-800 number.
The actual error message usually indicates pretty specifically if the issue is with the number type not being allowed in the field or a number that is not actually from the chosen country, which would be helpful in terms of understanding what specific validation issue you've run into here.
The attached patch resolves the issue by changing the scheduled email datetime format to ISO 8601, which includes timezone information at the end. WebformScheduledEmailTest is also updated to assert the new format is coming out as expected in messages.
chrisolof → created an issue.
TR → credited chrisolof → .
Notes from testing this MR against real traffic for about two weeks now:
This is currently performing very well / as designed against real traffic. The addition of the ASN lookup on those requests where it is actually necessary (requester not openly identifying as bot and not already blocked at the visitor-level) is so fast, even without the optional C extension, that it is imperceptible to our end-users. On the other hand, rate-limiting all crawlers, including those that horizontally spread out across multiple IPs and/or user agent strings, is a perceptible performance boost for our end-users.
Here's an initial patch that makes rescheduling after webform / source entity updates optional. Needs review.
chrisolof → created an issue.
I like this idea. I don't think number is the right type (we'd lose the excellent input format flexibility we have today), but I see no downsides to switching to tel. It sounds like it would have client-side UX benefits and supports all phone number formats (assuming we don't specify a pattern).
I think this also opens up some neat future possibilities. The patch looks fine but needs testing.
Also, related, over in 🐛 Undefined array key "extension" in /src/Element/PhoneNumber.php on line 237 Needs review I'm proposing we switch the extension sub-field type to number.
The attached patch resolves the issue in the affected environments.
I've made three changes in here:
1. Switch the extension sub-field type from text to numeric.
2. Remove the custom numeric extension validation code in Drupal\phone_number\Element\PhoneNumber::phoneNumberValidate() that is no longer needed.
3. Ensure the array key "extension" is actually set before trying to access its value in Drupal\phone_number\Element\PhoneNumber::phoneNumberValidate().
I suspect the third change is most responsible for eliminating the error here, and the patch could certainly be reduced to just that change. However while examining the validation of our extension sub-field it felt like a good opportunity to correct its field type and reduce our element's validation code a little bit. It could be this validation area was written before core had a numeric field type, or perhaps the original author was unaware of it. At any rate, this also enables client-side validation of the extension field, which is nice and something we've never had.
chrisolof → created an issue.
chrisolof → created an issue.
Looks good to me. Thanks Vaish!
Add Webform Element Values Limit under Enhancements section.
Merged latest 6.2.x changes into MR branch. Removed unused use statement. Added note about the default timezone that will be used to interpret the custom send-on date if not provided in the date string (so a source-code-dive isn't necessary to figure that out). Current state of the MR is attached, along with a screenshot of the changed field.
MR 425 opened. Status changed to needs-review. Proposed resolution and UI changes sections updated to match the resolution proposed in MR 425. Immutable diff (patch) attached here capturing the current state of this MR.
After diving into the code a bit more I've realized, rather than broadening out to accepting just ISO 8601 and MYSQL DATETIME formats, it probably makes more sense to accept anything PHP's strtotime() function can consume. The downstream code just throws this value against strtotime(), which accepts many formats, including both MYSQL DATETIME and ISO 8601. As such, limiting entry to just MYSQL DATETIME and/or ISO 8601 seems unwarranted and adds what feels like unnecessary complexity to the validation method.
The approach I'm taking with the MR currently is to drop the "ISO date" stuff and instead ask for a date or date-time string in a format compatible with PHP's strtotime() function, keeping the current MYSQL DATETIME YYYY-MM-DD or YYYY-MM-DD HH:MM:SS as the suggested format. This seems to fix the bug in the least disruptive way, bringing in actual ISO 8601 date format support, keeping MYSQL DATETIME support, slightly reducing validation complexity, and adding support for many other date formats.
I'll post the MR for review after I've confirmed tests pass locally.
chrisolof → created an issue.
chrisolof → created an issue.
Yes - confirming this looks great and works well. Thanks Vaish!
Excellent improvements here. The current state of the MR seems to work well and does not appear to be disruptive to existing installations.
Thanks everyone!
This looks good and works well. Thanks Vaish!
I think we should remove the 'country-code' part, since it seems to just duplicate the "country" part. With that change we would match up to what data parts exist at the field's schema. Maybe someday we can open up another property here to expose country name, but probably not necessary at this point.
chrisolof → created an issue.
I tried applying the patch from #22, but ran into errors due to the removal of Drupal\Core\Security\TrustedCallbackInterface. I think the removal of TrustedCallbackInterface was likely unintentional.
Attached is a re-roll of the patch in #11 against 10.2.x, with the requested post_update hook added so a cache clear is forced.
Adding interdif.
After posting I noticed the presence of the "Multiple entities" config option, and realized the prior patch could be improved by eliminating the canonical rerouting behavior (and hasMultiEntityPreviewLinks() checks) one level up when preview_link.settings:multiple_entities is FALSE. I think this will have a small performance benefit for sites that do not utilize multi-entity previews.
Attached is an alternative patch that attempts to resolve the issue here by restricting the canonical rerouting behavior to just multi-entity preview links, where it seems to serve a purpose.
It may make sense to go even further with another restriction: only apply the reroute behavior if the entity's latest revision is unpublished. That would allow this rerouting behavior on multi-entity preview links to drop out automatically as preview-versions of entities become published (and thus the preview link is no longer needed to see the latest version).
I'm also re-titling the issue to "Canonical reroute behavior is confusing & feels unnecessary in many situations", switching it to a bug report, and adding these alternate resolution ideas into the list of proposed resolutions.