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

Add Microsoft Edge User Agent #464

Merged
merged 9 commits into from
Mar 24, 2022
Merged

Add Microsoft Edge User Agent #464

merged 9 commits into from
Mar 24, 2022

Conversation

dutchie027
Copy link

@dutchie027 dutchie027 commented Mar 23, 2022

What is the reason for this PR?

This pull request adds the MS Edge User Agent.

  • A new feature
  • Fixed an issue (resolve #ID)

Author's checklist

Summary of changes

The Microsoft Edge User Agent was added in the User Agent Provider. It can generate an Edge UA for Windows, Linux, Mac and also iOS. To support iOS, the iOS device function was also added to the User Agent Provider.

A test was written to search for "Edg" when calling this user agent as that is the commonality among all Ednge UAs. (Edg, EdgA and EdgiOS respectively are the three different "distinct" Edge UAs.)

PR for documentation submitted

Review checklist

  • All checks have passed
  • Changes are approved by maintainer

@bram-pkg
Copy link
Member

Please revert the change in the Person provider. Otherwise this looks good.

@dutchie027
Copy link
Author

should I push the person change in another commit?

@dutchie027 dutchie027 changed the title Add Microsoft Edge User Agent and one additional name Add Microsoft Edge User Agent Mar 23, 2022
@pimjansen
Copy link

should I push the person change in another commit?

No we are not just randomly adding names since people want their own

@pimjansen
Copy link

Do we have some source refs on how these look for Edge? PR looks fine but have to cross check to be sure

@dutchie027
Copy link
Author

Here is my current user agent:

Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/99.0.4844.74 Safari/537.36 Edg/99.0.1150.46

The latest user agents for Edge

It seems that when the User Agent test reverted to the original name of `edge` and not `msedge`. This change fixes it back.
@pimjansen
Copy link

Ok thanks for the list, looks good. What still is missing is the addition in the Generator phpdoc. Since all calls to legacy are still made using a __call it is required.

@dutchie027
Copy link
Author

Copy. Didn't see that in the "Best Practices" area. Should be in Generator.php now for doc and autocompletion.

@pimjansen
Copy link

Copy. Didn't see that in the "Best Practices" area. Should be in Generator.php now for doc and autocompletion.

Yeah it is until we reach v2. Mostly because static type tools like psalm and phpstan complain if it is not.

Will give it a last review tomorrow but i think we are good to go. Maybe @bram-pkg finds some time earlier though

@bram-pkg
Copy link
Member

Fixed that last style error for you.

@bram-pkg bram-pkg merged commit acef8f2 into FakerPHP:main Mar 24, 2022
@dutchie027
Copy link
Author

Thanks. Switching between github.dev and trying to lint using a remote desktop connection makes for a lot of errors it seems :(

@bram-pkg
Copy link
Member

bram-pkg commented Mar 24, 2022 via email

@robbinjanssen
Copy link

robbinjanssen commented Apr 11, 2022

There is a bug in this PR, i've fixed it in #469

Update; a little more details would be nice: The static is called edge but the method is called msedge(). When calling a random userAgent it tries to call edge() which doesn't exist. The PR renames edge to msedge

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.

4 participants