🇺🇦Ukraine @abramm

Patch for applying via Composer.

abramm → created an issue.

Hi, not sure what you mean by 'For some reason' - can you please explain why the change is needed?

Hi @mably,

Welcome aboard! I've added you to the maintainers list.

The module is not commonly used nowadays since the domain module itself now provides an ability to switch the theme, however, I think it may be still useful to make a D11 release for those who are migrating older sites.

LGTM.
Keeping assigned to @mably for the release.

Yes, that's what I mean; no one has expressed an interest in this module since 2022.

Actually, I'd say it's no longer needed as we developed it years ago when Commerce didn't have BOGO.

We're currently not looking for a co-maintainer.

As for D10/D11 support, there's an automatic patch, but no one seems to have checked it - probably just no one is interested in that.

I don't think this is an issue.
The module assumes that import and export operations are done on the same site or between sites with identical configurations.

I've pushed a fix for the issue outlined in #3 (hope @ras-ben doesn't mind; otherwise, feel free to remove/squash my commit),
Attaching a patch to apply via composer.json.

The MR doesn't correctly account included and excluded dates; that's likely because include/exclude logic is handled in the recurring_events_event_instances_pre_create hook implementation (recurring_events_recurring_events_event_instances_pre_create_alter). This causes an issue with the removal of instances that are no longer in the range.

The proposed code change calls this hook only for events instances to be created while originally it was called for *all* instances.

Steps to reproduce:

1) Create the weekly event series, starting from e.g. the beginning of the year and ending at the end of the year.
2) Observe 51 instances (or whatever the number of weeks is in range) created.
3) Change the included date range to include only one month from the range, save the series.

Expected: all instances except those in the included range are deleted.
Actual: none of the instances are deleted.

Note: I'm talking about the Excluded dates and Included dates fields, which are below the normal range selection.

The change looks reasonable, although it'd be great if there would be a test and require-dev (as mentioned) before the merge.
Also, the patch should be changed to MR to allow tests run.

Here's an MR changing default behavior.

abramm → created an issue.

The proposed MR allows logging in with email stored in the username.

abramm → created an issue.

I checked the MR, added a minor suggestion.

Well, if the field type comes from something that's published on Drupal.org and is maintained, we could easily merge in the patch. Everybody wins.

Hi cnfl,

Plugin derivatives are not a single_content_sync specific feature; they are a Drupal way to use the same class for multiple plugins. You can learn more about derivatives in Drupal development docs. Due to this reason, it's not documented in the module.

We're using derivatives to use the same class for multiple field types plugins; e.g. it would be stupid to have separate classes, say, for string and number field types if their code would be 100% identical.

You don't necessarily need to use derivatives for your plugin; that depends on your the field type(s) you're working on. Is it a custom field type or something from Drupal.org ?

abramm → created an issue.

Hi @koosvdkolk,

You're right, the warning message is outdated; it references the very old hooks logic before we had plugins.
Feel free to submit the MR with a fix/README update.

abramm → changed the visibility of the branch 3202631-add-textarea-option to hidden.

Added two MRs (test only and test + code change), let's see if tests still passes.

abramm → changed the visibility of the branch 9.2.x to hidden.

abramm → changed the visibility of the branch 11.x to hidden.

The patch in #34 doesn't apply to 10.3, so re-rolled it against both 10.3.x (for those of us who need this patch to be applied to their sites) and 11.x branches.
Also, removed the unrelated change and split patches to test and test+fix.

Attaching a quick fix patch (also pushed to the MR branch).

abramm → created an issue.

Just spotted the edge case: the proposed RegExp solution will likely fail if there's a matching pattern inside the path (which is weird and I can't imagine anyone doing this, but technically it's still a valid URL).

const regex = new RegExp('(?<=//).*@(?=(?:.*.)+[^.]+)');

"https://valid:url@drupal.org/somewhere/".replace(regex, '');
// returns 'https://drupal.org/somewhere/' which is fine

"https://drupal.org/some//really:weird@url/".replace(regex, '');
// returns 'https://url/' which is not correct

Maybe an extra negative lookahead could handle this?

This change has caused a regression in our application.
I believe this is an edge case that's specific to our project (and a result of bad route argument naming), but I'm noting it here in case it would be helpful to anyone else.

We have a custom form controller and a route defined like this:

my_path:
  path: "/my-url/{some_param}"
  defaults:
    _controller: '\Drupal\my_module\Controller\CustomFormController::build'
    form_id: 'Drupal\my_module\Form\MyModuleFormClass'

Due to the change in this commit, the form_id may be now fetched from the request attributes (\Symfony\Component\HttpFoundation\Request::get), which caused the wrong form_id value to be fetched from the route attributes rather than from the request GET/POST parameters.

We've fixed this by changing the form_id argument to a different name (e.g. my_form_class) to avoid naming interference.

Having multiple exposed forms on the page, drupalSettings would be overwritten by the one rendering last; so I'd stick with the value from the data attribute.

Also, it'd be great if you could raise separate issues for two extra field types you add as they're not related.

Hi jannakha,

Thank you for your contribution!
Can you please also add tests? Please refer to an example here: ✨ Support geofield field type Fixed .

I don't think this is a module issue.

As an alternative, try using a development environment that correctly handles the installation of self-signed certificates, like ddev.

Hi Boobaa,

Thank you for your effort!
A few thoughts here.

1) We definitely don't want to have accessCheck(FALSE). The current user may not have permissions to view all revisions; this code will give them read access. This is a security/data disclosure issue.

2) The core version requirement should be core_version_requirement: ^9.3 || ^10, same as in root info.yml.

3) The following line will produce a warning if there are no revisions in YML file:
if ($content['revisions']) {

4) The revision creation time should be import/exported the same way we do for created/changed timestamps.

5) We'd likely need a test for this; also, please create the merge request to run automated tests.

Thanks!

The test is here, merged to 1.4.x.

I've added support for the address_country field type, it already merged to 1.4.x.
As for address_zone, there's an open MR adding support and it works, but needs tests before we could merge it.

Committed to 1.4.x.

Committed to 1.4.x.

Hi @ankondrat4,
Seems like we both pushed a similar code; I'll credit you as well.

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

Here's a simple MR fixing this.

abramm → created an issue.

Tests are failing due to the View Mode Selector module bug: 🐛 Warning: Undefined array key "view_mode" while creating the field Active .
Once it's fixed and tests passes, we should be good to go.

abramm → created an issue.

I think there's no need for a separate field processor plugin, it may be done via SimpleFieldDeriver.
Ref ✨ Support geofield field type Fixed for an example.

Here's a simple proof-of-concept patch.

abramm → created an issue.

abramm → created an issue.

I'd rather have this implemented with events.

Now when we have basic tests, we can implement support for additional field types.

As strange as it may sound, we can't rely on isStarted() to check if a user session exists. That's due to the session lazy load logic implemented in Drupal core.

An alternative is to check for the session cookie's existence (depending on implementation, it could be something other than a cookie).
Said, if there's no cookie, we don't want to lazy start the session (and so we won't send a new session cookie to the browser which is exactly what this issue aims to fix).

Attached patches are:
- just the test changes alone (had to simulate cookie existence);
- test + issue fix.

It seems like test failures are happening due to isStarted() returning FALSE if the session was started lazily.
The related core issue is #3044387: No way to tell if session has just started. → .

Just checked the code, and it should be easy to implement.
Also, this appears to be a duplicate of #3236416: OrderReceiptMail should honor order BCC even if not passed to send() → .

However, a UX question arises.
a) Should receipts copies be always resent to the BCC address? I don't think this is an option as it would change the behavior of existing sites on upgrades.
b) Or should there be a setting in order type (like a 'Send copies of re-sent receipts' checkbox next to the 'Send a copy of the receipt to this email' field in order type settings)?
c) Or maybe the checkbox could be added to the resend confirmation form/page.

I'd prefer option b) but it's up to Commerce maintainers to decide.

Oops, sorry, I've unintentionally hidden fork branches. Got them back again.

abramm → changed the visibility of the branch 3413901- to active.

abramm → changed the visibility of the branch 3413901-promotion-default-timezone to active.

abramm → changed the visibility of the branch 3413901- to hidden.

abramm → changed the visibility of the branch 3413901-promotion-default-timezone to hidden.

The issue is likely caused by the commerce_google_tag_manager module.
This issue seems relevant: 🐛 Fatal error when calculating shipping for shipments with no set amount RTBC .
Changing to Needs review for maintainers to confirm it's not Commerce core issue.

Probably something like https://www.drupal.org/project/error_page → could be used instead.
There are a lot of places in Drupal, Commerce, or any other software where unexpected errors may occur.
Patching just a payment process pane seems to be a very very very specific fix for a specific edge case.

Hi sioux,

What is the module used to build a slider? I've seen a 'revolution' method being called in your jQuery code so I suppose that's a revolution slider (but there are no D8/9/10 compatible modules at Drupal.org or I'm looking at the wrong place).

It seems like the module generates inline script tags which doesn't sound right.
Instead, I'd suggest moving slider instance settings to Drupal settings object and initializing instances with Drupal behavior JS.

Hi joekings,
Sorry if I haven't been clear, the PR is not ready for testing. It's just some basic sceleton code.

Hi valthebald,

I didn't test this yet but the code looks promising. We're going to work on the module issues at Global Contribution Weekend which is happening next week, will probably check it there.

Thank you for your effort!

We still support Drupal 9 on a best-effort basis, so it's definitely for 2.0 only.

That'd be a nice addition.
I would still ignore some field types though (like entity references) as they could be unsafe to import/export without special handling.

I've pushed an initial boilerplate for the Comments entity type support class.
A challenging part would be exporting references to the parent Node (which should already exist in the system) and a reference to the parent comment (which should already exist as well).

As an option, we could add a setting to export the Node with all of its child Comments, but that could not work if there're a lot of comments for the node.

Keeping this open for now.

What a nice day.

You're right, comments aren't currently supported. I'll check what's needed to add support.

abramm → created an issue.

We might still want to have this issue even if you're not willing to work on it anymore, so I'll reopen it.

Hi,
I'm @paulrad's coworker and have a user role allowing opting projects into security advisory coverage.
I can assist with co-maintainership if needed.

Hi, a few questions here:

1) How is this different from 📌 Drupal 10 compatibility Needs review ? I don't think there's a need to have separate issues.
2) What versions of Drupal and Odoo did you test with?

Hi kevinquillen,

That's intentional. Since Drupal 9.1, the event name is not required. Instead, the event class name is used if it's not set.
See \Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch.
You can simply subscribe to ExportEvent::class, there's no need to maintain separate IDs constants for that.

I'm closing this issue as it works as designed, please reopen if you disagree. Thanks!

abramm → created an issue.

Hi nikhil_dawar,

Thank you for the patch!

Can you please create the MR? We have some tests and automatic checks that I'd like to see before we move forward.
Also I can see you've commented out a part of code; we don't keep dead code in repository so let's remove it altogether.

Here's the MR fixing this issue (and patch if anyone wants to apply it).

abramm → created an issue.

abramm → created an issue.

Hi cosolom,

Please don't change the status to RTBC as there are no community reviews; you've changed the status for your own code.

As for the approach you've taken, the problem is that you've fixed the symptom rather than root cause of the issue; the unique setting should be always present for the field. Not having it is likely a problem and the way you've fixed it is just by ignoring it.

I appreciate your patch and I'd like to keep it here so people could use it as workaround if they happen to have the same issue but unfortunately that's not the fix for a root cause.