Bug #76155

Fluid does not detect all xmlns namespaces

Added by Markus Klein almost 4 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Must have
Assignee:
Category:
Fluid
Target version:
Start date:
2016-05-11
Due date:
% Done:

100%

TYPO3 Version:
8
PHP Version:
Tags:
Complexity:
easy
Is Regression:
Yes
Sprint Focus:
Stabilization Sprint

Description

<div xmlns:f="..." xmlns:x="...">
  <div xmlns:r="..">asdf</div>
</div>

In the example above only the f and x namespaces are recognized, but r is missing.

The reason is the usage of a wrong variable in \TYPO3\CMS\Fluid\Core\Parser\PreProcessor\XmlnsNamespaceTemplatePreProcessor such that only the first found tag with a namespace definition is searched.

This HTML used to work in CMS 7 and I'm not aware of an according breaking change documentation.

Associated revisions

Revision 1bbac59a (diff)
Added by Claus Due over 3 years ago

[DOC] Document xmlns-carrying node behavior in Fluid

This change documents the fact that xmlns-based
namespace importing works slightly different since Fluid
standalone became the base. Migration docs included.

Change-Id: I58f7a7e09eb0b640514cc3bf885d6ed05ea988e4
Resolves: #76155
Releases: master
Reviewed-on: https://review.typo3.org/50245
Tested-by: TYPO3com <>
Reviewed-by: Benni Mack <>
Tested-by: Benni Mack <>
Reviewed-by: Susanne Moog <>
Tested-by: Susanne Moog <>

History

#1 Updated by Markus Klein almost 4 years ago

Ehm... we even have a unittest to enforce that??

'Only handle first tag with xmlns ViewHelpers found'

#2 Updated by Markus Klein almost 4 years ago

  • Sprint Focus deleted (On Location Sprint)

#3 Updated by Markus Klein almost 4 years ago

  • Assignee changed from Markus Klein to Claus Due

#4 Updated by Markus Klein almost 4 years ago

  • Description updated (diff)

#5 Updated by Claus Due almost 4 years ago

I wish this was quick to explain, but it isn't. There are plenty reasons why now, we only support a single "place" to register namespaces for each template file.

Where to begin...

Let me compare previous logic to current one:

  • Before, namespaces were held in the parser and every method to manipulate them was fluid-core-only. That's very different now; namespaces are decoupled from the parser and are now managed by the ViewHelperResolver.
  • Before, namespaces could be added and removed by the parser at will, since only a single namespace with any identifier was supported. That's also very different now: when you register a namespace twice you don't replace the old one - you extend that namespace to look in additional class namespaces.
  • Namespace extraction from xmlns is now done as a pre-processing step, no longer part of the parser as such. It would not be possible to extract our namespaces this way and still be able to tell the parser when to apply our extracted namespaces (which node? When to remove it again and how?)

Combine those and it gives us a situation where, if the templateparser was to perform such manipulations, we would have to remove said namespace again afterwards because it would have to expire once the enclosing xmlns-carrying tag is closed. That puts quite a bit of complex requirements on the way namespaces are stored - adding a namespace that's already registered and then removing that again would have bad side effects so we would have to somehow track much more about the namespace add/remove process.

We're further challenged by the extraction now sitting in the pre-processing step that's fired only once. Our extractions would have to require new, currently non-existing logic in the templateparser to support modifying namespaces at certain points based on nodes... I really would not go in that direction as the parsing step is already quite complex.

The best compromise that does not cause confusion is the one I chose, I believe. It is the one that works with almost all current implementations and doesn't pollute namespaces unexpectedly by combining all extracted namespaces regardless where they sit in the template's node tree. But it is a compromise.

And that's why there's a unit test for it (coming back to the actual subject). It is an intentional behavior and as such, it has a test to ensure that behavior happens.

You are also right there's no breaking change described in the change log... that's my fault - I forgot about it while writing the breaking changes, and I know why: I was very focused on the ViewHelper API and not so much on use cases that had no implementations in the TYPO3 core. Sorry about that, if there's a way to update that so long after the fact, let me know!

But in regards to restoring the old behavior I would personally vote not to attempt that. Anything we do in that regard would inevitably require that fluid itself gets changed significantly, making the parsing and coupling with pre-processors/renderingcontext even more complex, and it honestly feels like far too much. The right answer is imho that this edge case is no longer supported - our pragma now is we encourage globally registered namespaces (via view/viewhelperresolver) and extending namespaces once per physical template file or source chunk that gets parsed.

#6 Updated by Markus Klein almost 4 years ago

  • Complexity deleted (no-brainer)
  • Is Regression changed from Yes to No

Hi Claus!

Thanks a lot for the detailed explanation.
I absolutely have no problem with the breaking change. We just have to document it. Better now than never.
Most people will read the breaking docs only after the final CMS 8 release anyway.

Can you please create a patch, which adds this information?
I propose to add it to the existing RST. Please set Mattes and me as reviewers, so we make sure that the wiki (etc.) is updated then as well.

#7 Updated by Benni Mack almost 4 years ago

  • Target version changed from 8.2 to 8.3

#8 Updated by Markus Klein almost 4 years ago

  • Complexity set to easy
  • Is Regression changed from No to Yes
  • Sprint Focus set to Stabilization Sprint

#9 Updated by Benni Mack over 3 years ago

  • Target version changed from 8.3 to 8.4

#10 Updated by Markus Klein over 3 years ago

To clarify things: Just an RST is missing documenting the breaking change!

#11 Updated by Gerrit Code Review over 3 years ago

  • Status changed from Accepted to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/50245

#12 Updated by Gerrit Code Review over 3 years ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/50245

#13 Updated by Gerrit Code Review over 3 years ago

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/50245

#14 Updated by Gerrit Code Review over 3 years ago

Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/50245

#15 Updated by Anonymous over 3 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100

#16 Updated by Riccardo De Contardi over 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF