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

Deprecates and replaces the Gravatar helper with a simplified version… #147

Merged
merged 1 commit into from
Mar 24, 2022

Conversation

gsteel
Copy link
Member

@gsteel gsteel commented Feb 22, 2022

Q A
Documentation yes/no
Bugfix yes/no
BC Break yes/no
New Feature yes/no
RFC yes/no
QA yes/no

Description

The current Gravatar view helper has a tonne of setters and getters, dodgy inheritance and checks out superglobals to figure out if the current http request is secure or not.

The existing helper is deprecated completely in this pull with a replacement implementation that simply spits out an img tag with the relevant src.

Notable differences…

  • Uses protocol relative url for the src to avoid inspecting the request
  • Closes the img tag with /> regardless of whether the current doc is XHTML/HTML5

Because this was originally part of #146 - it imports the psalm type AttributeSet from HtmlAttributesSet which is added in #146 so psalm qa will fail.

Maybe this pull can be left for when/if 146 is merged and then rebased so psalm passes.

@Ocramius
Copy link
Member

Needs another skim before review: CI failing 😁

@gsteel gsteel changed the base branch from 2.20.x to 2.21.x March 23, 2022 14:46
@Ocramius Ocramius added this to the 2.21.0 milestone Mar 24, 2022
@Ocramius Ocramius self-assigned this Mar 24, 2022
… that achieves the same goal.

Signed-off-by: George Steel <george@net-glue.co.uk>
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Ocramius Ocramius merged commit b7c269a into laminas:2.21.x Mar 24, 2022
@froschdesign
Copy link
Member

@gsteel
Updates for documentation are missing:

@Ocramius
Copy link
Member

@froschdesign I would leave all notices completely out, for stuff that can be deprecated statically. Notices are only necessary for combinations of input parameters.

@froschdesign
Copy link
Member

@Ocramius I mean the notice in the documentation: https://docs.laminas.dev/laminas-view/helpers/gravatar/

@gsteel
Copy link
Member Author

gsteel commented Mar 24, 2022

Sorry @froschdesign - #158 is ready for review :)

Would you be able to add recently merged pulls to the v3 project under done? This will help me when it comes to going over the deprecations/removals for the migration guide.

@gsteel gsteel deleted the gravatar branch March 24, 2022 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants