Better data types on apache::vhost parameters #2251
Conversation
There was a problem hiding this comment.
Aside from the comment below these changes look fine to me.
You do have some syntax errors however and will need to make some updates to the unit tests to account for the changing types.
I would also prefer it if you where to update the example on line 1412 to show $headers as an array
They now both only accept arrays of non-empty strings. The default is set to an empty array, which behaves as if it's not set.
This was previously filtered out but it's best if the information simply never is entered in the first place. The include is also simplified and only applied if the vhost is present.
My bad. I rely on CI to run that for me.
That's an interesting point because the example really has nothing to do with the parameter it's used with. It passes in the |
|
Looking at the examples closer, that seems to be a common trait, that it shows the variable being passed as part of |
|
@ekohl Took a quick look at the spec test failures and they all seem to be from |
Ideally a Struct would be used instead of Hash for $rewrites, but this is a closer description of the real data type.
This also pulls in mod_jk if needed.
|
Made it an array, hopefully the tests now pass. |
|
Still getting some failures it seems, and strange ones |
|
I'm not sure what's up with the tests now. They appear to be stuck? |
|
Not sure what is going on, have tried to rerun the tests and it keeps happening, think there may be a corrupted file in the action(?). |
ekohl
changed the title
|
@ekohl Have tried rekicking a few times and closing and opening the pr but the failures are still there. |
|
Let's see if #2252 does work. If not, I wonder if there's something that's running into a loop or something. Worst case I'll split up the PR since they're multiple commits anyway. |
They now both only accept arrays of non-empty strings. The default is set to an empty array, which behaves as if it's not set.