Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make wms layer groups requestable #5371

Closed
wants to merge 3 commits into from
Closed

make wms layer groups requestable #5371

wants to merge 3 commits into from

Conversation

tbonfort
Copy link
Member

@tbonfort tbonfort commented Jan 17, 2017

In the current layer grouping implementation, a layer with wms_layer_group=/g will only let its group "g" appear as requestable (i.e. with a <name>g</name> in capabilities) if there is also a LAYER in the mapfile with NAME=g.

I'd argue that this is unintuitive as when you are creating a hierarchy of layers you end up being obliged to define dummy/empty layers in order to get that hierarchy to be actually useful. e.g. see https://github.com/mapserver/mapserver/blob/rel-7-0-4/msautotest/wxs/wms_inspire.map#L257

The WMS spec (wms 1.1 - 7.1.5.4) does provision for the case where a group of layers appears but cannot be called if it has only a <title> defined but not a <name>. This is what we are implementing currently in the capabilities, but we are not respecting it in GetMap requests (we happily render the group even if it has no <name> defined).

In order to be coherent with the concept of layer grouping, be coherent with our actual behavior, and simplify writing grouped mapfiles, I propose we modify the current behavior and always add a <name> tag to group layers.

  • patch to add <name> tag to group layers
  • capabilities test
  • request test
  • documentation update
  • HISTORY.TXT update
  • migration guide update

@tbonfort tbonfort requested a review from Schpidi January 17, 2017 08:37
@tbonfort
Copy link
Member Author

cc @yjacolin

@tbonfort
Copy link
Member Author

see also #5220

@yjacolin
Copy link
Contributor

yjacolin commented Feb 2, 2017

@tbonfort do you need a feedback from me?

@jmckenna jmckenna self-requested a review February 9, 2017 18:38
@sdlime
Copy link
Member

sdlime commented Aug 23, 2017

Is this one ready to merge?

@yjacolin
Copy link
Contributor

If we can help in any way, let's us know.

fredj added a commit to fredj/mapserver-debian-package that referenced this pull request Sep 21, 2017
@ebelo
Copy link

ebelo commented Dec 12, 2017

Hi, is this ready to merge? Can we do something so that it gets into master?

@yjacolin
Copy link
Contributor

@tbonfort I am working on the doc parts of this feature. I created a new branch up-to-date with master and cherry-pick your 3 commits, added an item in HISTORY.TXT. Are you ok to close this PR and I will open a new one with a link to the doc. Thank for you answer.

@yjacolin
Copy link
Contributor

MIGRATION_GUIDE.txt doesn't need to be updated.

@yjacolin
Copy link
Contributor

following discussion in Mapserver-dev list, a new PR superseed this one: #5533

@rouault rouault closed this Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants