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

[#11926] Changed view engine service closures to no longer receive th… #13839

Merged
merged 4 commits into from
Feb 24, 2019
Merged

[#11926] Changed view engine service closures to no longer receive th… #13839

merged 4 commits into from
Feb 24, 2019

Conversation

dschissler
Copy link
Contributor

@dschissler dschissler commented Feb 18, 2019

…e dependency injector as the second parameter.

Hello!

  • Type: bug fix
  • Link to issue: #11926

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the Contributing Guidelines?
  • I have checked that another pull request for this purpose does not exist.
  • I wrote some tests for this PR.

Back in Phalcon 2.0 the DI was changed to bind service functions to itself. Before that it was necessary to use use ($di) in the function. This is a minor tweak to remove the DI from being passed as the second argument in the view engine service functions. Instead use $this.

NOTICE: I'm trying something different by using a different forked branch for each PR. If this change is acceptable then I'll make the change log entry to get this all of the way through. In order to avoid merge conflicts this should be done as late as possible. I hope to be able to have several of these going at once for higher throughput.

@dschissler dschissler closed this Feb 19, 2019
@dschissler dschissler reopened this Feb 19, 2019
@dschissler dschissler closed this Feb 19, 2019
@dschissler dschissler reopened this Feb 19, 2019
@codecov
Copy link

codecov bot commented Feb 19, 2019

Codecov Report

Merging #13839 into 4.0.x will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           4.0.x   #13839      +/-   ##
=========================================
+ Coverage   66.2%    66.2%   +<.01%     
=========================================
  Files        451      451              
  Lines      89787    89792       +5     
=========================================
+ Hits       59443    59447       +4     
- Misses     30344    30345       +1
Impacted Files Coverage Δ
ext/phalcon/mvc/view/simple.zep.c 50.6% <0%> (-0.27%) ⬇️
ext/phalcon/mvc/view.zep.c 46.99% <0%> (+0.07%) ⬆️
ext/phalcon/mvc/view/engine.zep.c 33.33% <0%> (+3.92%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b19d84a...852a594. Read the comment docs.

@dschissler

This comment was marked as abuse.

Copy link
Contributor

@sergeyklay sergeyklay left a comment

Choose a reason for hiding this comment

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

LGTM

@sergeyklay sergeyklay added breaks bc Functionality that breaks Backwards Compatibility and removed breaks bc Functionality that breaks Backwards Compatibility labels Feb 23, 2019
@dschissler

This comment was marked as abuse.

@sergeyklay
Copy link
Contributor

I will keep this open for a day or two to allow other contributors a chance to speak up, and to take a fresh look at it later. But at the moment it seems good to me.

@niden niden merged commit 49ce804 into phalcon:4.0.x Feb 24, 2019
@niden
Copy link
Member

niden commented Feb 24, 2019

Thank you @dschissler

@sergeyklay
Copy link
Contributor

@niden Could you please add this to the docs

@dschissler dschissler deleted the 11926 branch March 10, 2019 00:11
@niden niden added the documentation Documentation required label Apr 9, 2019
@niden niden added 4.0 enhancement Enhancement to the framework and removed documentation Documentation required labels Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to the framework
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants