Item8903: INCLUDE will return entire topic if pattern does not match

pencil
Priority: Normal
Current State: Closed
Released In: 1.0.10, 1.1.0
Target Release: patch
Applies To: Engine
Component:
Branches:
Reported By: JohannesHammersen
Waiting For:
Last Change By: KennethLavrsen
If you do
%INCLUDE{"WebHome" pattern="^.*?(WILL NOT MATCH).*"}%

and your WebHome does not contain the text "WILL NOT MATCH" it will include the entire WebHome

Might have to do with: http://trac.foswiki.org/browser/branches/Release01x00/core/lib/Foswiki.pm#L1987

-- JohannesHammersen - 13 Apr 2010

On 1.1, the problem is in Foswiki::Macros::INCLUDE::applyPatternToIncludedText() :
        $text =~ s/$pattern/$1/is;

I suggest that should be changed to:
        # The trailing () ensures that $1 is defined if the pattern matches
        # for the case where $pattern does not contain (...)
        if ( $text =~ /$pattern()/is ) {
            $text = $1;
        }
        else {
            $text = ''; # but see "Warning if pattern does not match" below
        }

That will allow us to dispense with the leading and trailing .* in INCLUDE's $pattern most (if not all) of the time, whilst existing patterns that do have leading and trailing .* will continue to work.

There could potentially be uses of INCLUDE with $pattern that might break. All those that I have though of rely on undocumented behaviour:
  1. Existing, undocumented behaviour: If the pattern does not match then the whole topic is returned
  2. Existing, undocumented behaviour: If the pattern omits leading (or trailing) .*, then the portion of the topic before the pattern (or after, if trailing .* is omitted) will also be included.

I view both of these aspects of the existing behaviour as bugs.

Warning if pattern does not match
INCLUDE warns if the topic does not exist. I think it should also warn if the pattern does not match. warn="off" should disable the warning.

Kenneth - I added you to the "waiting for" list so that you are notified of this patch

-- MichaelTempest - 05 Jul 2010

Both $pattern in SEARCH and INCLUDE are very goofy and geeky.

I have not get come to terms with what works and what does not work.

But I have noticed and taken advantage of using ^ as the start of the $pattern in SEARCH. But I never managed to get anything to work if it did not end with a .* and that is so strange.

Especially in SEARCH with multiple="on" the scope of $pattern should be the one line of text found, and then I never understood why I always had to end with .*

So I am all for improving this. But watch out for the beginning of the string. We do not want to break using ^ to match beginning of line.

-- KennethLavrsen - 15 Jul 2010

I changed INCLUDE to return blank if the pattern does not match in 1.1.

TODO: Make INCLUDE warn if the pattern does not match

-- MichaelTempest - 16 Jul 2010

Including a part of another topic and returning nothing is a feature.

It could be that you want to include user comments from another topic and if there are none you want none.

So the warning - which is a good tool for someone trying to get a regex to work - will be a problem.

INCLUDE has the warn parameter where the warning is either on or off or a predefined message. So INCLUDE should only warn when warn is not off. And we should not break the current way warn works. So maybe we already have the feature?

There are two different cases. Either the included topic does not exist. Or the topic exists but the pattern does not match.

I am not sure how warn works today when the pattern does not match. I would assume it just returns the entire topic and does not warn as this report says which is clearly a bug. So I would say it is OK to simply let the current warning feature work.

Maybe in 1.2 we can add a warn="debug" feature that can be more specific telling the application builder what is goind on. We could use same feature for SEARCH.

-- KennethLavrsen - 17 Jul 2010

The warning for pattern-does-not-match will work the same as for topic-does-not-exist

-- MichaelTempest - 17 Jul 2010

It looks like this isn't working on trunk.foswiki.org :/
%INCLUDE{"Sandbox.WebHome" pattern="^.*?(WILL NOT MATCH Oh PURPLE ewe).*"}%
No permission to view Sandbox

-- MichaelTempest - 20 Jul 2010

It seems there was a problem with the updating trunk.foswiki.org. Now it works smile

I cannot see a good way to add a warning now without changing the interface to the IncludeHandlers. I doubt that would be a good idea after a feature freeze. I raised Item9385 so that the need for a warning is not lost. For 1.1, INCLUDE will return blank (empty string) if the pattern does not match.

-- MichaelTempest - 24 Jul 2010

I am back-porting the change to 1.0.x branch, so this change will also go into 1.0.10

-- MichaelTempest - 01 Aug 2010
 
Topic revision: r20 - 08 Sep 2010, KennethLavrsen
The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License    Legal Imprint    Privacy Policy