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

[4.0] Small refactor of typehint check in CMSPlugin #30907

Merged
merged 3 commits into from
Oct 8, 2020
Merged

[4.0] Small refactor of typehint check in CMSPlugin #30907

merged 3 commits into from
Oct 8, 2020

Conversation

SharkyKZ
Copy link
Contributor

@SharkyKZ SharkyKZ commented Oct 3, 2020

Summary of Changes

Follow up to #30581.
Renames poorly named checkTypeHint() method and makes it private.
Changes it to accept ReflectionParameter instead of ReflectionType to maintain the whole parameter logic inside.

Testing Instructions

Test that Joomla and plugins still work.

Documentation Changes Required

No, unless the method was documented somewhere.

@adj9
Copy link

adj9 commented Oct 3, 2020

I have not tested this item.

With PR enabled the plugins are active and do not remain. I logged out and logged in from the administrative panel and there are no anomalies.

With these tests, can I confirm the PR?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30907.

@SharkyKZ
Copy link
Contributor Author

SharkyKZ commented Oct 3, 2020

Yes, as long as it's working as before it's fine.

@ceford
Copy link
Contributor

ceford commented Oct 4, 2020

I have tested this item ✅ successfully on 1f6b0b4

I applied th patch and checked Joomla still works - it does. I put a die; in the new function to check it is being called - it is. All good.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30907.

@particthistle
Copy link
Member

I have tested this item ✅ successfully on 1f6b0b4

Tested, Joomla continued working.

Hard to find a plugin that the test would either fail for before the test. Harder to find one that would fail for Joomla 4 after the test without writing one. Plugins where parameters are not typehinted I would imagine are unusual?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30907.

@alikon
Copy link
Contributor

alikon commented Oct 6, 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30907.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 6, 2020
@laoneo laoneo merged commit 25f8d40 into joomla:4.0-dev Oct 8, 2020
@laoneo
Copy link
Member

laoneo commented Oct 8, 2020

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 8, 2020
@laoneo laoneo added this to the Joomla 4.0 milestone Oct 8, 2020
@SharkyKZ SharkyKZ deleted the j4/impr/event-typehint branch October 8, 2020 10:19
sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020
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.

7 participants