Admins please read this #212
Comments
|
Thanks for opening all of these issues and your willingness to create pull requests for it! There is a lot of interesting information here, but I can also see some errors. And there may also be some fundamental differences to the approach this library is using. Take for example the Maple browser on Samsung television devices. I'm conflicted on how to detect this. Maple is an internal code name for the browser, not a consumer facing name. It was always labeled as "Web Browser". To me that is a big distinction. What also not helps it that Samsung sometimes refers to Maple as a development platform (for HTML based smart tv apps) and apps build using that development platform sometimes also use the token Maple in the UA string. So far I've not shown the name here, but used Maple as an identifier that we are dealing with a Samsung television device. Or take the Sony PSP browser. I know it is a NetFront based browser. But it can not be detected as "NetFront", because it is a browser build in house by Sony itself, based on NetFront technology and source code. It is not a NetFront browser released by the Access. I am also hesitant to call it the "PSP Browser" or something to that effect, because on the PlayStation itself it is labeled "Internet Browser". And "Internet Browser" is too generic to be used as a browser name for detection purposes". I want to land as much of this as possible, but I do have to go over each individual item. I'm sure most can be landed as it, but some of the issues you've opened are not as straightforward as they seem. So please, continue opening issues and making PR's, but I am hesitant to land all these changes as one big PR. Relatively safe changes, like new browsers could be bundled together in one PR. But please use different commits in the PR for the different browsers. And please be aware that any change also requires that you add examples of the UA string to the test suite. Any change that does change the detection of existing UA strings should definitely be in a separate PR, because it affects backwards compatibility. Thanks again! |
|
Hi, Just to let you know I've finished adding all the missing browsers. I have manually tested each one. Fingers crossed you can merge them all. Any changes or issues please tag me, because our company is using this package and when I was adding the browsers I was updating our api to match each one. Many thanks, Ayumi. |
|
Thank you. I’ll go over the PRs. Given the huge number of them it may take a bit of time, but I think most of them can be merged without any issues. |
|
As soon as those PRs are merged and @ayumi-cloud work is done I will replicate the change in Parser-JavaScript. Thanks for the leg work here! |
|
@NielsLeenheer @mariotsi Hi, hope all is well, just wanted to ask. Would it be a good idea to merge all these pr's into a separate branch before adding them to the |
|
Perfect!
…On 30 Aug 2020, 12:43 +0100, Ayumi ***@***.***>, wrote:
Just to let you know, I'm going to be finishing off coding a large project in September and then either near the end of September or in October, I will finish off coding and updating all the pull requests and issues I have opened up in this repo.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Finished coding all the pr's I needed to add to this repo now. Two issues I leave for you guys to code are: #255 and #253 as I'm not sure how to write the solutions for these? Any issues with the pr's tag me and I will update the pr's accordingly. Please can you add a new semantic version when all the pr's are merged so I can back test this repo for any issues etc. Thanks. |
|
I was wondering when the next batch of pull requests will be merged? |
|
Just to let you know, I have now finished updating and creating pr's for this repo. Sorry to have created so many pr's I was coding my application at the same time as this repo and testing this repo as I was coding both repo's at the same time and fixing any problems that failed testing. Please go ahead and start merging these pr's whenever you got free time. Please tag me with any issues or any Thanks, Ayu. |
|
Thank you @ayumi-cloud . Sorry for the late response. Looks like @NielsLeenheer is currently busy. As soon as he check and approves the changes I'll apply them on the Parser-Javascript version. |
|
@mariotsi No problem, just tag me and I will keep a look out. p.s. Travis no longer works anymore and needs to be moved over to Github Actions. |
|
I would be very grateful if you could create a new repo titled: Link: https://github.com/ayumi-cloud/Parser-Laravel (We're using Laravel and created this wrapper which I would prefer if it got maintained over here as an official wrapper). Thanks. |
|
My apologies for being really slow with responding. As @mariotsi mentioned, I am really busy at the moment and will look at all the changes in the coming weeks. @ayumi-cloud thanks so much for your patience so far. I will probably have some time next week and I hope to be able to publish a release version before the years end. I'll also look at Travis and the Laravel wrapper. |
|
I've processed a whole batch of PRs. Some as is, some slightly changed. I did use a clean commit for each of them, to get rid of unrelated files and changes in the original PR. This way I could process the changes much faster instead of having to rebase the PRs and revert some unrelated changes. Unfortunately this is all I got time for during the Christmas break. I do hope to find some more time in the coming weeks and will look at the remaining PRs. For now, I will release an updated version with all of changes so far. Thanks! |
|
It's now been over a year since I created the out standing pull requests. Sorry I can't wait any longer and decided to merge all the pull requests on a separate repo. I have rebased every pull request with the master and double checked all the code and updated the pull requests with your suggestions. People are welcome to download and test out all the merged pull requests here: https://github.com/summercms/sc-parser-module/releases Any issues let me know and I will try and update the pull requests in both repos. Please note that I've got cancer right now and need to start chemo in the coming weeks - so I maybe slow to reply and answer. (I needed to merge and get this work done before I focus on my health). Notes for admins: Didn't add: Repo has Chrome 90 and this pr has chrome 88 (not needed): #647 Added: These two pr's are the exact same, so added just one of them: https://github.com/WhichBrowser/Parser-PHP/pull/532/files and #137 Add Security: Fixed https://github.com/summercms/sc-parser-module/issues/224 PHP now requires: |
and others
Hello,
Sorry to mass open many issues, we have been running various tests against your repo on many browsers and see if your package can detect them or not?
We haven't finished yet and still running some tests and will open a few more issues up (probably)?
We just going through this list of browsers right now found here and running tests: https://udger.com/resources/ua-list#Browser
So far your package is finding around 70% of the browsers - so good work.
Please can you tag me or message back when you are free and we can start working on a bunch of pull requests for you - to update this package and make it up to date. (It seems like it detects old browsers well and newer browsers not so well).
Lastly, we have put regex and notes on each issue to help with the pull requests. There are a few issues that we need to talk to you about versions and a few complicated fixes.
But we think we can code 98% of all the opened issues without any help.
We intend to add the UA's to the testing folder as well.
Kind regards,
Ayumi.
@NielsLeenheer
@mariotsi
The text was updated successfully, but these errors were encountered: