Problem/Motivation

ValueError: DOMDocument::loadXML(): Argument #1 ($source) must not be empty in DOMDocument->loadXML()

The main issue occurs in the removeDefaultNamespaces function:

https://git.drupalcode.org/project/feeds/-/blob/8.x-3.x/src/Component/XmlParserTrait.php?ref_type=heads#L115

If the XML payload is large, the server may require additional memory to process it due to PCRE limits.
When the regex fails, the resulting XML string may be empty, which leads to the
DOMDocument::loadXML() exception.

Steps to reproduce

  1. Import or process a large XML file using Feeds.
  2. Ensure the XML contains default namespaces (xmlns=).
  3. Trigger the removeDefaultNamespaces logic.
  4. Observe a regex backtracking limit error leading to an empty XML string.
  5. Note the resulting DOMDocument::loadXML() exception.

Proposed resolution

Before attempting to remove default namespaces, explicitly check whether the XML
contains xmlns=. Run the regex conditionally and validate the regex execution.
If a PCRE backtracking limit error occurs, throw a clear runtime exception with the
regex error code instead of returning an empty XML string.

See:
https://www.php.net/manual/en/function.preg-last-error.php

Remaining tasks

N/A

Issue fork feeds-3563946

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

foxy-vikvik created an issue. See original summary.

foxy-vikvik’s picture

foxy-vikvik commented
Issue summary: View changes

foxy-vikvik opened merge request !233

megachriz made their first commit to this issue’s fork.

megachriz’s picture

megachriz
Primary language Dutch
commented

Thanks for your contribution! I fixed a RuntimeException class not found error in the code and added test coverage. Since I wasn't sure how to write a test for this case, the test was generated by AI and then slightly adjusted by me.

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Active » Needs review

Setting to "Needs review" so others have a chance to review the code if they want.

foxy-vikvik’s picture

foxy-vikvik commented
Issue summary: View changes

kksandr made their first commit to this issue’s fork.

kksandr’s picture

kksandr commented
Issue summary: View changes
Related issues: +#3552353: ValueError: DOMDocument::loadXML(): Argument #1 ($source) must not be empty in DOMDocument->loadXML(), +#3421301: ValueError: DOMDocument::loadXML(): Argument #1 ($source) must not be empty

The method was updated to handle any PCRE error instead of checking only for PREG_BACKTRACK_LIMIT_ERROR, so unexpected regex failures are no longer silently ignored and NULL from preg_replace() cannot slip through unnoticed. The exception now includes the PCRE error message and code for clearer diagnostics.

As a follow-up, @megachriz, what are your thoughts on simplifying the test by using a static XML fixture instead of dynamically generating XML to trigger the PCRE error? If package size is a concern, a .gitattributes file with export-ignore could be used to exclude all development-only files from release archives. What do you think?

megachriz’s picture

megachriz
Primary language Dutch
commented

Yes, having a static XML file instead of a generated one is fine to me for the tests.

I do think returning the XML earlier (when there is no xmlns) is better for the readability of the code, because it requires less nesting.

I wasn't aware of the feature to exclude certain files from releases. That would make the Feeds download a lot smaller, because there are many files related to tests.

kksandr’s picture

kksandr commented

I do think returning the XML earlier (when there is no xmlns) is better for the readability of the code, because it requires less nesting.

That's fair - this is just personal preference. Feel free to refactor it with early returns if you prefer that style.

kksandr’s picture

kksandr commented
Related issues: -#3421301: ValueError: DOMDocument::loadXML(): Argument #1 ($source) must not be empty
kksandr’s picture

kksandr commented

The suggested changes have been implemented. @megachriz, could you please review them when you have a chance? Thank you!

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs review » Fixed

The updates to the test look wonderful and easier to read. I've scheduled a merge for this fix. Thanks all for contributing!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.