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

Converting PHP-FastCGI handler from a managed handler to a module mapping #308

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andy1547
Copy link

@andy1547 andy1547 commented May 5, 2017

Resolves: #305


This change is Reviewable

@andy1547 andy1547 changed the title Converting PHP-FastCGI handler from a managed handler to a module map Converting PHP-FastCGI handler from a managed handler to a module mapping May 5, 2017
@Devvox93
Copy link

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 161 at r1 (raw file):

                Modules = $ModuleType
                ScriptProcessor = $Path
                ResourceType = 'File'

Why hardcoding this to File?
I suggest leaving it out or adding the parameter $ResourceType.


Comments from Reviewable

@Devvox93
Copy link

Devvox93 commented Aug 29, 2017

I'm about to make bigger changes to IISModule (and other resources), so I have added a comment to Reviewable.
I'd like this issue to be Done before I start.

I can resolve my comment myself too, as the author hasn't been active on this for a few months now.
Is it okay to pass the review then? Asking because that's usually a bad practice and I don't want to just mess up the system.
I will make the resource use a new parameter as I suggested in my comment as part of the bigger changes mentioned before.

@tysonjhayes
Copy link

Just to make sure I'm following you want to basically incorporate this change into your PR? Personally I'm fine with whatever as I don't fully comprehend this change.

@Devvox93
Copy link

Devvox93 commented Aug 30, 2017

Exactly. The changes the author meant to perform was changing "Module" to "Modules" (with an S), which fixes a bug. However, author also added a hardcoded ResourceType, which is kind of dirty and as part of my changes I'd add a Parameter for setting the ResourceType. The question if I could pass the "dirty" change as I'll be soon replacing that anyway.


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@tysonjhayes
Copy link

I'm fine with you doing the change in yours and closing this one. Let's give another day or two for @andy1547 to respond before moving forward. If he doesn't respond I'll close this PR.

@andy1547
Copy link
Author

andy1547 commented Aug 30, 2017

@Devvox93 I agree that ResourceType should be a string parameter with a ValidationSet of Unspecified (don't think this is an option in the UI but is in handler documentation), File, Directory and Either. The IIS UI does not force the user to input this and defaults to File, so I'd suggest using that as the default.
Would also need to add ResourceType to GetScript and validate in TestScript.
@tysonjhayes I won't be able to do these changes any time soon so by all means supersede them, however for the time being I'd suggest keeping this PR open.

@Devvox93
Copy link

With the UI defaulting to File (I didn't think about that), I suggest this PR to be approved, as it does fix the bug mentioned in issue #305 . The extended functionality of using a Parameter (also in Get and Test) can be added later in my PR.


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@Devvox93
Copy link

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 161 at r1 (raw file):

Previously, Devvox93 wrote…

Why hardcoding this to File?
I suggest leaving it out or adding the parameter $ResourceType.

Default value that UI sets is File, so it's not that ugly. Future change will use a parameter.


Comments from Reviewable

@Devvox93
Copy link

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@dsccommunity dsccommunity deleted a comment from msftclas Sep 26, 2017
@dsccommunity dsccommunity deleted a comment from msftclas Sep 26, 2017
@johlju
Copy link
Member

johlju commented Apr 23, 2018

@Devvox93 you solution sounds good. Do you have a PR ready for this change? If that includes this change then I suggest we close this PR. @andy1547 I will set this a abandoned, but if you want to continue the work on this than, then please let me know.

@johlju johlju added the abandoned The pull request has been abandoned. label Apr 23, 2018
@johlju johlju changed the base branch from dev to master December 30, 2019 12:20
@johlju johlju changed the base branch from master to main January 7, 2021 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned The pull request has been abandoned.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xlisModule Uses wrong configuration key/value pairs when adding handler config
7 participants