🇺🇦Ukraine @abramm

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.

I've added tests infrastructure, a few tests for simple field types (string, boolean), multivalued fields, complex field type (datetime_range) and contrib module field type (address).

This should cover a major part of functionality; adding all remaining field types from SimpleFieldDeriver could be good entry-level tasks for Contribution Weekend.

abramm → created an issue.

We're not your personal developers army @Shikha_LNweb, I'd be happy to review the MR adding the feature you need if you'd submit one.

There's a module configuration page (/admin/config/content/single-content-sync) which has a setting for entity types allowed for export.

Can you please check if your entity types are allowed for export there?

Images inserted via entity_embed aren't supported right now. The entity_embed is a third party contrib module which does inline images processing in its own way.

This is not a bug, so I'm changing this to feature request; we might support this in future if someone would be interested in implementing it.

This looks like you're using the entity_embed module; can you please confirm that?

Hi Shikha_LNweb,
Can you please provide the example HTML content?

I don't think this was fixed in f060e4cf, in fact, f060e4cf has introduced the code which I'm fixing in the patch.

From f060e4cf:

      if (empty($webforms[$webformId])) {
        $webforms[$webformId] = [];
      }
      if ($webforms[$webformId][$webformRevisionId]) {
        continue;
      }

As you can see, the second if statement does not have !empty. This is in 2.x-dev branch.

What's strange is there another commit (b8869ca96e338935d9557a12b4bc7de9624e4c41) in a different branch (2.0.x-dev) which adds the same code to the trait but with a different commit message and it does have !empty. Not sure what happened there, maybe some git rebase and force-push magic, but I'm happy to conclude this issue is not happening in 2.0.x-dev branch which is currently used to build releases. Likely the project where I faced this issue was using 2.x-dev branch.

Thanks for a quick test!
I'll handle the rest, just wanted to make sure I guessed the correct reason for this.
Not sure about timing but I'm planning to get into this someday this week.

Had a quick look in the code, it is possible that the issue is happening if the field is referencing non-existing file.

From FileAsset.php:

      $file = $file_storage->load($item['target_id']);

      $file_item = [
        'uri' => $file->getFileUri(),
        'url' => $file->createFileUrl(FALSE),
      ];

Just as a quick test, can you please add the following line after $file = $file_storage->load($item['target_id']); (line 114 in FileAsset.php)?

if (!$file) { continue; }

Note this is not a correct fix, just a quick check to see if I guessed correctly.

Ah, so the #3123959 is already merged!
Didn't notice that; sorry for confusion!

#7 is the correct patch then.

NOTE: The patch in #7 seems to be a re-rolled version of #3 which is NOT a correct patch for this issue and contains a fix for a different issue as well; the patch in #3 was provided for convenience.

The correct fix for this issue is still #2.

abramm → created an issue.

Hi @lostcarpark,

The 'unsafe new static' is a known issue reported by PHPStan for many Drupal project; in fact, core CI configuration even ignores it. That's because this is in fact not an issue but rather a best practice in Drupal world. Updating the code the way PHPStan suggest would actually make it worse for Drupal use case.

Read here on why the issue is happening and how to handle it:
https://www.drupal.org/docs/develop/development-tools/phpstan/handling-u... →

TL;DR: Just put a phpstan.neon file like this: https://git.drupalcode.org/project/single_content_sync/-/blob/1.4.x/phps...

Opened the MR with quick fix, ready for review.

abramm → created an issue.

This issue is also happening with Drush's PHPStorm metadata generation.

I'm running Drush 12.2.0.0, running the following command causes failure:

drush generate phpstorm -y

I'm not copying the stack trace as it's pretty much same as in previous reports.

This should be enough for now, I think this covers most critical importer parts.

I'd appreciate another set of eyes just to double check I didn't miss anything important.

Pushed some few basic tests to start with.

abramm → created an issue.

There are now 2.0.0 and 2.x-dev releases compatible with Drupal 10.

Updated issue summary to clearly state that this issue is happening if there's no composer.json file committed in a module repository.