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

Integrate bugfix from 5.0.2 #320

Closed
wants to merge 4 commits into
base: develop
from

Conversation

Projects
None yet
4 participants
@Mte90
Copy link
Collaborator

Mte90 commented Dec 28, 2018

Description

#301

This merge 4 tickets that are bugfixes from 5.0.2, one of them fix errors for PHP 7.3

@Mte90 Mte90 changed the title 5.0.2 Integrate bugfix from 5.0.2 Dec 28, 2018

@nylen

This comment has been minimized.

Copy link
Member

nylen commented Dec 28, 2018

Hi, this isn't going to work as it currently stands. The way this is set up is almost impossible to review and verify that these are actually things we want.

What bugs are we fixing here? Each one needs to go into a separate PR with an explanation and links to relevant Trac tickets.

The most urgent type of fix from other WP branches is PHP 7.3 compatibility. I am not sure if we even want to accept other things right now.

Also:

  • Please don't modify the original commit messages, just pull them directly over using git cherry-pick. I don't know what "merge 44284" means, it doesn't provide any information about the change.
  • Please read https://forums.classicpress.net/t/backport-wp5-trac-tickets/110/6 which has some explanation of how we can manage these changes. We need links to the Trac changeset and the WP commit on GitHub.
@Mte90

This comment has been minimized.

Copy link
Collaborator

Mte90 commented Dec 30, 2018

I followed the guide on discourse, for the title this happened because I merged 4 commits that require a manual fix because of conflicts in the code.
THe ticket #301 ticket cover the 4 tickets that are merged, the commits in this pr include the changeset included (that has a different name).
I made a single pr because the patch included are part of 5.0.2 and include parts that involve also us.

@nylen

This comment has been minimized.

Copy link
Member

nylen commented Jan 4, 2019

I followed the guide on discourse

No, not really :) Please include the original commit messages, and links to the original commits on both Trac and https://github.com/WordPress/wordpress-develop.

I made a single pr because the patch included are part of 5.0.2 and include parts that involve also us.

Creating a PR with multiple unrelated changes and no explanation of what is being changed is not a good idea. Someone who wants to review this PR cannot know what it is about, how to test it, or whether the changes are something we want to accept. If some changes are good, but not others, there is no way to sort that out because they are all bundled together.

I'm closing this PR. Please open a new one for each specific change and include an explanation of what you are changing and why, with links to the original change in WP. I'd suggest starting with the PHP 7.3 fix.

@nylen nylen closed this Jan 4, 2019

@Mte90 Mte90 deleted the Mte90:5.0.2 branch Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment