RFE: Mapscript ms_newMapObj constructor which loads from string

[project10]

A request for enhancement to MapScript (PHP):

• Add a new MapObj constructor, which mimicks the functionality of the msLoadMapFromString() C function

This may be a duplicate of bug #2083.

Example/expected usage:

$mapFileString = "MAP ... END" $mapObj = ms_newMapObjFromString($mapFileString);

Which would be equivalent to writing $mapFileString to a mkstemp() file and passing the file path to the existing constructor.

[sdlime]

Reassigning to the PHP/MapScript component.

Steve

[dmorissette]

This is kind of related to ticket #2298 which in turn refers to #2143, the implementation of RFC-31 (​http://mapserver.gis.umn.edu/development/rfc/ms-rfc-31) in which the ability to update specific objects from string was added.

Note that RFC-31 has not been implemented in PHP yet (see ticket #2298 about this)

Steve, does RFC-31 include a method to create a new mapObj from a string? I didn't see that in a quick browse of the code.

[sdlime]

RFC-31 does include a function to load from string (msLoadMapFromString() in mapfile.c). I could have sworn I added that method to Swig/MapScript in map.i but apparently not. It does work as I have test cases that use that function so maybe I forgot to commit or perhaps we weren't sure how to best implement. All the updateFromString methods do exist in Swig/MapScript. I wish we could overload constructors for a mapObj...

At any rate, looking at mapserver.h I see that msLoadMapFromString() is being wrapped so code like:

$map = mapscript::msLoadMapFromString($buffer, $path);

does work with SWIG/MapScript. I don't know what that means for PHP, can you mimic that?

Steve

[mgleahy]

a patch that adds ms_newMapObjFromString($string,$path);

[mgleahy]

I don't know if anyone is doing any work on this (nobody replied on the dev list), but with some help from a friend to get me started, I figured I could give it a shot. I've attached a patch that seems to have done the trick for me - a quick test in my application shows maps loading from text strings without any trouble. I don't know if I might have missed anything that could cause problems, or if it's too late to get this in for the 5.2 release...but it would be ideal for me if something like this could be added.

[sdlime]

I was waiting for the PHP folks to chime in... ;-)

I had another idea yesterday that would rely on just a single constructor. In SWIG the constructor is:

    mapObj(char *filename="") 
    {
        if (filename && strlen(filename))
            return msLoadMap(filename, NULL);
        else { /* create an empty map, no layers etc... */
            return msNewMapObj();
        }      
    }

We could change things to inspect the input string and load as a string accordingly. In that case the constructor would become:

    mapObj(char *filename="") 
    {
        if (filename && strlen(filename))        
            /* 
            ** Would need a robust test here, might also test for # since
            ** a trailing comment like "# end test.map" could trip things
            ** up. 
            */   
            if(strrstr(filename, ".map") || strrstr(filename, ".MAP")
              return msLoadMap(filename, NULL);
            else
              return msLoadMapFromString(filename);
        else { /* create an empty map, no layers etc... */
            return msNewMapObj();
        }      
    }

I do think it's too late for 5.2, plus there's a bit of uncertainty in how to implement. If you can get by with your hack of PHP/MapScript for the moment I can promise inclusion in 5.4 or perhaps even 5.2.1 if folks are comfortable with a change like that.

Steve

[dmorissette]

Too late for 5.2 unfortunately... let's try to get this in early in 5.4.

Steve, when we work on this let's not forget that the new function needs an optional path arg as in the attached patch so that relative paths inside the mapfile can work.

[mgleahy]

Steve: I had also made another patch that kind-of did what you suggest, but instead it used a third argument for mapObj (int from_text), which would load the map from a text string if equal to MS_TRUE, and otherwise it would load from a filename as before. This makes loading from text more deliberate, and would avoid having to detect the filename - but it requires corresponding udpates to php_mapscript.h and php_mapscript.c. The way you suggest requires less work.

This RFE raises another question though: is there a similar way to get a map object back into text without having to be written to a file first? As far as I can tell, there isn't an option...but if there is, that would also be useful.

Mike

[mgleahy]

Ok, here's what I've got in mapscript_i.c, and it seems to be working pretty well without having to modify any other files:

mapObj *mapObj_new(char *filename, char *new_path) {
  if(filename && strlen(filename))
  {
    ms_regex_t re;

    if(ms_regcomp(&re, MS_DEFAULT_MAPFILE_PATTERN, MS_REG_EXTENDED|MS_REG_NOSUB|MS_REG_ICASE) != 0) {
      msSetError(MS_REGEXERR, "Failed to compile expression (%s).", "msEvalRegex()", MS_DEFAULT_MAPFILE_PATTERN);
      return msNewMapObj();
    }

    if(ms_regexec(&re, filename, 0, NULL, 0) != 0) { /* no match */
      ms_regfree(&re);
      return msLoadMapFromString(filename, new_path);
    }

    ms_regfree(&re);
    return msLoadMap(filename, new_path);
  }
  else { /* create an empty map, no layers etc... */
    return msNewMapObj();
  }
}

I'm not sure which of the cflags would be most appropriate be used for ms_regcomp (apart from MS_REG_ICASE, which I'm guessing makes it case-insensitive?). At any rate, presumably we could make an expression with this that looks for newline characters, and/or a comment at the end of the file. Or perhaps as a rule require that loading a mapfile from text must have MAP/END at the start/end of the string.

I'm still wondering about writing the map object back out to text though - would it be particularly complicated to make a version the msSaveMap function that returns a string instead of writing to a filename? At a glance, it looks relatively doable...

[sdlime]

Mike, what is the value of MS_DEFAULT_MAPFILE_PATTERN? I like this as an approach!

Writting to a string is doable. It was talked about breifly when the string reading work was done. The mapfile writing process could be streamlined a good bit and writing to a buffer could be done in the process.

Steve

[mgleahy]

In mapserver.h:

#define MS_DEFAULT_MAPFILE_PATTERN "
.map$"

This is ok, but has the same caveat you mentioned before - any string ending in '.map' will be interpreted as a mapfile, even if it's a comment at the end of a text string. Also, does mapserver trim filename strings? If so, then any filename that has trailing space would be treated as text. Another option is to use something like '^{MAP.*END$' as the expression to determine if the string is text rather than a filename.}

Another idea I had was to check test whether file exists, and if not, attempt to load the mapfile from string...but I don't have the experience/time to figure that out at the moment - plus it might be confusing if somebody uses the wrong filename.

[dmorissette]

Call me old fashioned, but I still prefer being explicit, i.e. different method or contructors arguments for filename vs map data as opposed to magically passing everything through the same arg and implementing some auto-detection that risk failing.

[sdlime]

I hear you. I'd prefer overloaded constructors but that won't work since the input arguments are the same in the string and filename cases. Having a stand-alone loadMapFromString function seems real hackish to me. That's why I like this approach better in abscence of something else. The documentation would make things clear, you pass either the mapfile file name or the mapfile content as a string... My 2 cents.

Steve

[mgleahy]

Just to add another 2c; my preference is weighted somewhat by the time it would take to actually have this functionality in a released version. If soon (say version 5.2.1), then I don't really care which approach is taken...but if it's going to take much longer than that (e.g., 5.4, as is the current milestone), then I'd rather use a separate function...or at least some way that I can test for the presence of the functionality.

E.g., rather than crashing ms_newMapObj and handling errors after the fact, I could check function_exists("ms_newMapObjFromString") before trying to load from a string - if it doesn't exist, then I can write the string to a file and load it the usual way. It's not a major issue for me at this point, but it might be a bit more important if I'm to make my application portable to other servers (e.g., windows, where I've never successfully compiled my own mapserver executables).

Just a thought anyway. At any rate, I've been using the REGEX approach for a while now, and haven't run into any problems - loading from both file and string.

[sdlime]

Replying to mgleahy:

Just to add another 2c; my preference is weighted somewhat by the time it would take to actually have this functionality in a released version. If soon (say version 5.2.1), then I don't really care which approach is taken...but if it's going to take much longer than that (e.g., 5.4, as is the current milestone), then I'd rather use a separate function...or at least some way that I can test for the presence of the functionality.

E.g., rather than crashing ms_newMapObj and handling errors after the fact, I could check function_exists("ms_newMapObjFromString") before trying to load from a string - if it doesn't exist, then I can write the string to a file and load it the usual way. It's not a major issue for me at this point, but it might be a bit more important if I'm to make my application portable to other servers (e.g., windows, where I've never successfully compiled my own mapserver executables).

Just a thought anyway. At any rate, I've been using the REGEX approach for a while now, and haven't run into any problems - loading from both file and string.

Do you have a patch that demonstrates the co-mingled solution? I could apply in the trunk and let folks play...

Steve

[dmorissette]

Mike, since that's a new feature and not a critical bug fix that can't go in 5.2.1 and would have to go only in 5.4.

Adding a ms_newMapObjFromString() method is my preferred approach and the patch that you attached to the ticket seems perfect for that. I'll assign to Alan here to test it and I'll apply it in SVN if all is good.

[aboudreault]

Tested and it work properly. Thanks mgleahy.

[sdlime]

IMHO I still like the shared constructor approach. The syntax for PHP and Swig MapScript are different at this point plus it feels hackish...

Steve

[dmorissette]

None of the automated filename-vs-mapbody tests proposed in this ticket seem to work for all possible cases... if you want to craft something for SWIG mapscript then we'll port it to PHP after.

[mgleahy]

Lets ms_newMapObj load mapfiles from text strings (if the string does not appear to be a path to a *.map file)

[mgleahy]

FWIW, there's the patch for using the shared constructor approach (basically, it's just the code from my comment above).

Hope this helps.

[mgleahy]

I noticed discussion on the dev list about releasing 5.4. Is there any way we can look at including one of these patches in that?

I've been patching every installation I have with mapObjFromString.patch - I decided after a while I liked it better than the REGX approach, mostly because it was easier to detect whether it was possible at all to create the object from a string (by checking for the presence of the constructor function). The REGX approach worked fine, but I think the separate constructor is least likely to cause any confusion/bugs for others with existing applications.

[dmorissette]

We seem to have a problem agreeing on the approach to use, that's why nothing has been committed to trunk yet. In the interest of doing something now, I'd suggest we add a mapObjFromString() to 5.4... I don't think that would hurt or prevent adding a shared constructor later on if someone can come up with a good one and deprecate mapObjFromString() at that time.

Alan, can you please take care of that?

[aboudreault]

I've commited the mapObjFromString() method to SVN trunk in r8420 and updated the documentation in r8421. I'll let the ticket open and discuss with Daniel about the shared contructor.

[dmorissette]

Let's close this as fixed for now. If/when someone works on a shared constructor then a new ticket should be created that will apply to both SWIG and PHP MapScript.