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.0] Tag no longer uses static methods #12379

Closed
wants to merge 1 commit into from
Closed

[4.0.0] Tag no longer uses static methods #12379

wants to merge 1 commit into from

Conversation

SidRoberts
Copy link
Contributor

No description provided.

@SidRoberts SidRoberts changed the title Tag no longer uses static methods. [4.0.0] Tag no longer uses static methods Oct 30, 2016
@sergeyklay sergeyklay added this to the 4.0.0 milestone Oct 31, 2016
@yankos
Copy link
Contributor

yankos commented Nov 10, 2016

Hm... Is it good?
Tag as class with static method is not ideal. But this PR do not bring something new.
I think much better if Tag will be as service.

@Jurigag
Copy link
Contributor

Jurigag commented Nov 10, 2016

Yea, don't like creating new tag object in each method.

@sergeyklay
Copy link
Member

sergeyklay commented Nov 10, 2016

@Jurigag Just inject Tag to the Di and use $this->tag, $di->get('tag'), etc. And yes, I agree with @yankos about service

@Jurigag
Copy link
Contributor

Jurigag commented Nov 10, 2016

Yea i know, but it should happen then in this PR everywhere i think. As well this would require Tag in factorydefault perhaps.

@yankos
Copy link
Contributor

yankos commented Nov 10, 2016

@Jurigag of course Tag must be in factorydefault.

So, maybe it's time to refactor Tag as service for 4 version of Phalcon?
I can to do this.

@SidRoberts
Copy link
Contributor Author

Static methods are glorified functions with namespaces. Using them in this way means we can't override them with a custom tag class and they're hard dependencies. It defeats the whole point of object-oriented programming. For all intents and purposes, they're effectively evil. :p

This PR is still a work in progress and I plan to get the 'tag' service from the DI instead of just creating a new instance every time. Unfortunately the PR doesn't pass in its current form so I'm focusing on that first.


let output = "",
html = "",
joinedContent = "";

let tag = new Tag();
Copy link
Member

Choose a reason for hiding this comment

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

@SidRoberts Can we seek it in the Di first?


if !collectionName {
let collection = this->getCss();
} else {
let collection = this->get(collectionName);
}

return this->output(collection, ["Phalcon\\Tag", "stylesheetLink"], "css");
let tag = new Tag();
Copy link
Member

Choose a reason for hiding this comment

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

The same


if !collectionName {
let collection = this->getJs();
} else {
let collection = this->get(collectionName);
}

return this->output(collection, ["Phalcon\\Tag", "javascriptInclude"], "js");
let tag = new Tag();
Copy link
Member

Choose a reason for hiding this comment

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

and here

@niden niden added this to In progress in 4.0.0 Release via automation Dec 10, 2018
@niden niden self-assigned this Dec 10, 2018
@niden niden mentioned this pull request Dec 11, 2018
3 tasks
niden added a commit that referenced this pull request Dec 11, 2018
niden added a commit that referenced this pull request Dec 11, 2018
niden added a commit that referenced this pull request Dec 11, 2018
niden added a commit that referenced this pull request Dec 11, 2018
niden added a commit that referenced this pull request Dec 11, 2018
niden added a commit that referenced this pull request Dec 11, 2018
niden added a commit that referenced this pull request Dec 11, 2018
niden added a commit that referenced this pull request Dec 11, 2018
niden added a commit that referenced this pull request Dec 11, 2018
niden added a commit that referenced this pull request Dec 11, 2018
niden added a commit that referenced this pull request Dec 11, 2018
niden added a commit that referenced this pull request Dec 11, 2018
niden added a commit that referenced this pull request Dec 11, 2018
niden added a commit that referenced this pull request Dec 11, 2018
niden added a commit that referenced this pull request Dec 11, 2018
@niden
Copy link
Sponsor Member

niden commented Dec 11, 2018

Addressed in #13656

@niden niden closed this Dec 11, 2018
@niden niden moved this from In progress to Done in 4.0.0 Release Dec 11, 2018
@SidRoberts SidRoberts deleted the v4-tag-static branch June 7, 2019 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
4.0.0 Release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants