Skip to content

WinHttp.WinHttpRequest.5.1 usage#512

Open
mlipok wants to merge 4 commits into
Danp2:masterfrom
mlipok:WinHttp.WinHttpRequest.5.1
Open

WinHttp.WinHttpRequest.5.1 usage#512
mlipok wants to merge 4 commits into
Danp2:masterfrom
mlipok:WinHttp.WinHttpRequest.5.1

Conversation

@mlipok

@mlipok mlipok commented Feb 27, 2024

Copy link
Copy Markdown
Contributor

Pull request

Proposed changes

Today I hit a problem when trying to update geckodriver where InetRead() returns with error

Checklist

  • I have read and noticed the CODE OF CONDUCT document
  • I have read and noticed the CONTRIBUTING document
  • I have added necessary documentation or screenshots (if appropriate)

Types of changes

  • Bugfix (change which fixes an issue)
  • Feature (change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (functional, structural)
  • Documentation content changes
  • Other (please describe)

What is the current behavior?

Sometimes InetRead and InetGet can return error thus there is no way to update driver with this udf.

What is the new behavior?

when InetRead returns error then WinHttp.WinHttpRequest.5.1 is used.

Influences and relationship to other functionality

none

Additional context

https://www.autoitscript.com/forum/topic/209621-inetget-alernative/
https://www.autoitscript.com/forum/topic/209610-inetget-dont-download-some-pages-since-ie11-is-no-longer-supported-by-some-sites
https://www.autoitscript.com/forum/topic/209472-download-problem-with-inetget/

System under test

FireFox

@sven-seyfert

Copy link
Copy Markdown
Contributor

Hi @mlipok , since you use the same code block ...

If @error Then
	Local $oHTTP = ObjCreate("WinHttp.WinHttpRequest.5.1")
	$oHTTP.Open("GET", $sURL, False)
	$oHTTP.SetRequestHeader("User-Agent", "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:121.0) Gecko/20100101 Firefox/121.0")
	$oHTTP.Send("")
	$sName = $oHTTP.ResponseBody
EndIf

... three times (for $sResult, $sData and $sDriverLatest), it would maybe be a good improvement to extract this code into a separate "internal usage" function 😀 .

Best regards
Sven

@mlipok

mlipok commented Feb 28, 2024

Copy link
Copy Markdown
Contributor Author

I expected such request.
Will do it ASAP.

@sven-seyfert

Copy link
Copy Markdown
Contributor

No pressure @mlipok , thanks for your stable contribution actions on this project 👌 .

@mlipok

mlipok commented Feb 29, 2024

Copy link
Copy Markdown
Contributor Author

Done: added _WD_DownloadAsBinary

@sven-seyfert

Copy link
Copy Markdown
Contributor

Cool, thanks @mlipok . I try to test it in the next day(s). Because I believe, the @error marcro could be a problem - but it's only a wild guess and feeling, no real statement. Let me test it 🤝 .

Comment thread wd_helper.au3 Outdated
Comment thread wd_helper.au3 Outdated
Comment thread wd_helper.au3
Comment thread wd_helper.au3
Comment on lines +2474 to +2475
; #FUNCTION# ====================================================================================================================
; Name ..........: _WD_DownloadAsBinary

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the function header should be ; #INTERNAL_USE_ONLY# ====== and the function should be __WD_DownloadAsBinary instead of _WD_DownloadAsBinary. Actually it does not matter, but it would fulfill the consistency 😇 .

@mlipok mlipok Feb 29, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have any reason to change _WD_DownloadAsBinary into internal usage only then you should try to apply the same reason to _WD_DownloadFile and see if this reason also makes sense here.

If you say that _WD_DownloadFile save data to file I would say that the destination does not matter here as I always try to avoid writing data to files on disk. Instead, I process the data in memory and write to database.

This is because of security reason (i.e. GDPR law) - I simply try to minimize the number of places where data leaves a trace (files on disc).

Summary: if any user of this UDF needs to use the function for download and writing data to a file, then the function of writing data to memory without writing it to disk is equally necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood and fair enough @mlipok 👍 .

Then at least the README.md file also has to be adjusted, right?
I mean the additional function should be described in the README like the other functions too.
Is this usually made by @Danp2 ? I believe he do a "Prep release" commit which will contain such things, right? Or do we as PR authors also adjust these files 🤔 ?

Best regards
Sven

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this usually made by @Danp2 ? I believe he do a "Prep release" commit which will contain such things, right? Or do we as PR authors also adjust these files 🤔 ?

@Danp2
What you think about ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Danp2 can you look here ?

@Danp2 Danp2 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't yet read through the past discussions on this PR, but I do have a few questions to add --

  • Do we need to support both InetRead and WinHttp downloads?
  • Is there functionality from WinHttp.au3 that could be used here instead of WinHttp.WinHttpRequest.5.1?

@sven-seyfert

Copy link
Copy Markdown
Contributor

Is there functionality from WinHttp.au3 that could be used here instead of WinHttp.WinHttpRequest.5.1?

This is actually a really good question @Danp2. Interesting.

@mlipok

mlipok commented Mar 2, 2024

Copy link
Copy Markdown
Contributor Author
  • Do we need to support both InetRead and WinHttp downloads?

Do not know.

@mlipok

mlipok commented Mar 2, 2024

Copy link
Copy Markdown
Contributor Author
  • Is there functionality from WinHttp.au3 that could be used here instead of WinHttp.WinHttpRequest.5.1?

I think yes.
But I'm not familiar with winhttp.au3 udf.

@mlipok

mlipok commented Mar 3, 2024

Copy link
Copy Markdown
Contributor Author
  • Do we need to support both InetRead and WinHttp downloads?

Do you mean to stop using InetRead in favor of WinHTTP functions ?

@mlipok

mlipok commented Mar 3, 2024

Copy link
Copy Markdown
Contributor Author

@Danp2 Danp2 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to stop using InetRead in favor of WinHTTP functions ?

Yes, that is what I was wanting you to consider.

@sven-seyfert

Copy link
Copy Markdown
Contributor

Few thoughts of mine:

After all these months, I lost the track of this PR. I lost the need or use case to switch from InetRead to a WinHTTP.au3 function. I also didn't read in the forums newer requests regarding this topic. I saw your mentioned links @mlipok, but no newer questions.

That's why I think about to leave it as it is at the moment.
But sure, this is only my thoughts - in general - no need, no action.

What do you folks think @Danp2 and @mlipok?

Best regards
Sven

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.

3 participants