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.
I just ran into this odd default behavior while evaluating the module. It looks like it's useful (perhaps needed?) when granting a multi-entity preview via a single link. More details in #3155009: Grant access to multiple entities by using a single preview link → .
For single-entity preview links, however, this redirection behavior feels like a bug. I would expect to see the preview-version through the preview link and the regular version through the regular link.
I'm tempted to provide an alternative patch that leaves this redirection in place for multi-entity preview links, but removes it for single-entity preview links where it seems unnecessary and confusing.
chrisolof → created an issue.
I just ran into this bug while attempting to display ECK entities in a views block using the swiper formatter views plugin. The changes in MR 2 fix the issue for me. Thanks @bbu23!
Attached is an immutable patch snapshot of MR 2, as it stands today.
Looks great - thank you for your contribution! This has been merged into our 1.0.x branch and will be part of our next release.
The latest patch (in comment #8) has been merged into our 2.x branch and is included in the latest 2.0.0 release. Thanks for the help everyone!
This has been merged into our 2.x branch and is included in the latest 2.0.0 release.
This has been merged into our 2.x branch and is included in the latest 2.0.0 release.
The attached patch adds scrollbar support into the module. Default settings match to API defaults. Empty checking is in place to avoid the necessity of an update hook to backfill existing swiper template config. I think the addition of an update hook here would be an improvement, but this is a start.
chrisolof → created an issue.
The attached patch adds basic mousewheel support.
It also removes some unnecessary trailing whitespace from the edited files (this was done automatically by my editor).
I almost went with an update hook to back-fill the new config parameter, but seeing no prior update hooks / pattern of this in the module, I decided instead to just handle the situation in the form via a null coalescing operator.
chrisolof → created an issue.
This is really neat. The flexibility to add multiple types of each property (say a work and home phone number, for example) and configure them all independently per your unique requirements immediately struck me as a nice improvement over the previous, fairly hard-coded model.
I haven't done a full implementation test against this branch yet, but after some light testing and code review I spotted a few potential improvements we might want to make at the early stages here:
- Omit rendering of empty property lines. I believe the prior setup accomplished as much via conditional output in the template. I just pushed this change into the issue fork.
- I suspect the render() method in the Photo plugin is not necessary, given the render method in VcardPropertyPluginBase.
- I wonder if supportsHomeWorkType might be better held in the annotation config / plugin definition...
- It seems to me that the plugin interface could drop prepareOutputString(), as it seems to be more of just a helper-method in VcardPropertyPluginBase, with nothing outside of VcardPropertyPluginBase depending on it. The render method seems like the one to keep in the interface since it's what the rest of the system hooks up to for output from the plugin. It's possible a plugin could decide to render by other means (or more directly), without following the prepareOutputString() then render() model.
- Document return values on prepareOutputString() and render(). I took a stab at this in my push to the issue fork since the empty-checking was related.
Again, I haven't done extensive testing, but the basics appear to be in place and working here. I would say this still needs work, especially given the lack of an upgrade path, and it probably deserves a major version change when it lands in a release, but it is definitely all coming together here.
Oh this is very cool! I'll try and get you a review here next week. I really like the new plugin design.
Just pulled in the latest 3.x and tested. It does appear to be working - thank you!
New patch attached, created against the latest 3.x branch code.
Oh bummer. I was incorrectly assuming this module was open to complete v4.0 property support.
At any rate I've already created a patch to add NOTE property support, so I'll post it here for your consideration. Maybe it will help others with similar needs.
Thank you for the work-around advice.
chrisolof → created an issue.
The attached patch reduces the required properties to just full name (FN), leaving everything else optional.
chrisolof → created an issue.
Patch attached.
chrisolof → created an issue.
The attached patch against 2.x adds the missing config schema file.
chrisolof → created an issue.
Confirming that the patch in #5 fixes the bug in my tests on a PHP 8.1 environment.