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.

Sign up for GitHub

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

Jump to bottom

[RTM] Improve the findByIdOrAlias() method #729

Merged
merged 1 commit into from
Apr 28, 2017
Merged

[RTM] Improve the findByIdOrAlias() method #729

merged 1 commit into from
Apr 28, 2017

Conversation

Toflar
Copy link
Member

@Toflar Toflar commented Mar 20, 2017

No description provided.

@Toflar Toflar mentioned this pull request Mar 20, 2017
Closed
@leofeyer leofeyer added this to the 4.4.0 milestone Mar 20, 2017
aschempp
aschempp suggested changes Mar 20, 2017
View reviewed changes
Copy link
Member

@aschempp aschempp left a comment

Choose a reason for hiding this comment

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

Good start, but there are a lot more cases I can find:

  • tl_page::generateAlias
  • tl_page::addAliasButton
  • ArticleModel::findByIdOrAliasAndPid

Also, we should probably change the other bundles, as the same rules should apply to news alias etc. in my opinion.

@Toflar
Copy link
Member Author

Toflar commented Mar 20, 2017

tl_page::generateAlias
tl_page::addAliasButton

How is this related to this pull request?

Also, we should probably change the other bundles, as the same rules should apply to news alias etc. in my opinion.

They all share the Model class as parent so automatically make use of these changes?

@aschempp
Copy link
Member

How is this related to this pull request?

You're right, I just quickly searched for alias=? 🙈

They all share the Model class as parent so automatically make use of these changes?

Unfortunately, that's only partially true: https://github.com/aschempp/contao-news-bundle/blob/825cc0992692ace816d213d5c8d66b51697b3c03/src/Resources/contao/models/NewsModel.php#L176-L176

@Toflar
Copy link
Member Author

Toflar commented Mar 20, 2017

Yeah sure, there are other methods that could be optimized.

@leofeyer leofeyer self-assigned this Apr 21, 2017
@leofeyer leofeyer changed the title Improve the findByIdOrAlias() method [RTM] Improve the findByIdOrAlias() method Apr 21, 2017
@leofeyer leofeyer merged commit a855bbd into contao:develop Apr 28, 2017
leofeyer added a commit that referenced this pull request Apr 28, 2017
@leofeyer
Improve the findByIdOrAlias() method in the other models (see #729).
f45ced2
@leofeyer
Copy link
Member

Improved the other methods in f45ced2. @Toflar /cc

@Toflar Toflar deleted the improve-idoralias branch April 28, 2017 09:24
leofeyer added a commit to contao/calendar-bundle that referenced this pull request Apr 28, 2017
@leofeyer
Improve the findByIdOrAlias() method (see contao/core-bundle#729).
ce1afe3
leofeyer added a commit to contao/faq-bundle that referenced this pull request Apr 28, 2017
@leofeyer
Improve the findByIdOrAlias() method (see contao/core-bundle#729).
2013251
leofeyer added a commit to contao/news-bundle that referenced this pull request Apr 28, 2017
@leofeyer
Improve the findByIdOrAlias() method (see contao/core-bundle#729).
85e456b
leofeyer added a commit to contao/newsletter-bundle that referenced this pull request Apr 28, 2017
@leofeyer
Improve the findByIdOrAlias() method (see contao/core-bundle#729).
a683104
@leofeyer leofeyer mentioned this pull request Nov 26, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aschempp aschempp aschempp requested changes

@leofeyer leofeyer leofeyer approved these changes

Assignees

@Toflar Toflar

@leofeyer leofeyer

Labels
feature
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
3 participants
@Toflar @aschempp @leofeyer