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

Html\Tag::prependTitle() and Html\Tag::appendTitle() inconsistency with Tag counterparts #14039

Closed
scrnjakovic opened this issue May 4, 2019 · 4 comments

Comments

Projects
3 participants
@scrnjakovic
Copy link
Contributor

commented May 4, 2019

As per changelog, Phalcon\Tag is to be replaced with Phalcon\Html\Tag, yet some of their methods have not only different signatures, but completely different implementations.

While that is not an issue itself, the way above mentioned methods behave in Phalcon\Html\Tag is wrong.

Phalcon\Tag::prependTitle(var title) implementation

cphalcon/phalcon/Tag.zep

Lines 919 to 933 in 1d4e2f1

/**
* Prepends a text to current document title
*/
public static function prependTitle(var title) -> void
{
if typeof self::documentPrependTitle == "null" {
let self::documentPrependTitle = [];
}
if typeof title == "array" {
let self::documentPrependTitle = title ;
} else {
let self::documentPrependTitle[] = title ;
}
}

Phalcon\Html\Tag::prependTitle(array title) implementation

cphalcon/phalcon/Html/Tag.zep

Lines 1075 to 1083 in 1d4e2f1

/**
* Prepends a text to current document title
*/
public function prependTitle(array title) -> <Tag>
{
let this->prepend = title;
return this;
}

Phalcon\Tag implementation is better.

@niden

This comment has been minimized.

Copy link
Member

commented May 4, 2019

Nice catch. Thanks. We will align those shortly.

@niden niden added the Bug - Medium label May 4, 2019

@niden niden added this to To do in 4.0 Release via automation May 4, 2019

@niden niden self-assigned this May 4, 2019

@emiliodeg

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Why there are 2 ways to do the same? Phalcon\Tag should not be a Phalcon\Html\Tag proxy? implementing DI or maybe Phalcon\Tag should desappear in favor of Phalcon\Html\Tag?

@scrnjakovic

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@emiliodeg Phalcon\Tag is going away, as said earlier. It will, however, stay for a while for backward compatibility.

@niden niden referenced this issue May 8, 2019

Merged

T14039 tag html tag #14052

4 of 4 tasks complete

@niden niden moved this from To do to In progress in 4.0 Release May 8, 2019

niden added a commit that referenced this issue May 9, 2019

@niden

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Resolved in #14052

@niden niden closed this May 9, 2019

4.0 Release automation moved this from In progress to Done May 9, 2019

@niden niden added the 4.0 label Jun 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.