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

Revert "Compat: Upgrade admin notices to use Notices module at runtime" #12440

Closed
wants to merge 5 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Nov 29, 2018

Related: #11604
Closes #12243

This pull request seeks to effectively revert #11604 .

Notably, since this was included in published releases already, it has been done in such a way that it bumps the major versions of the edit-post and notices modules to reflect the fact that the revert is a breaking change.

Two commits from #11604 have been exempt from the revert (i.e. will remain in master):

These were not strictly relevant to the addition of admin notices compatibility. Ideally, they would have been proposed as separate changes. In omitting them, they reduce the number of changes included in this revert.

Testing instructions:

Verify the inverse of the testing instructions outlined in #11604.

@aduth aduth added Backwards Compatibility Issues or PRs that impact backwards compatability [Package] Notices /packages/notices labels Nov 29, 2018
Erroneous conflict resolution
## 1.1.0 (2018-11-20)

### New Feature

- The `createNotice` can now optionally accept a WPNotice object as the sole argument.
Copy link
Member Author

Choose a reason for hiding this comment

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

This never actually went through in #11604. See #11604 (comment) .

@youknowriad
Copy link
Contributor

Should we close this? An alternative has been merged right?

@aduth
Copy link
Member Author

aduth commented Dec 3, 2018

@youknowriad #12444 was a safer "revert", while not actually being a proper revert. The code still lingers. The changes here would be more an actual revert. The public-facing interface changes are limited to the speak option in notices, which we could realistically keep around. The AdminNotices component, while noted in the changelog of [email protected], isn't actually exposed on the public interface of the module.

@gziolo
Copy link
Member

gziolo commented Feb 5, 2019

#12243 is closed. It looks like the original issue was resolved. Does it mean that this PR can be closed, too?

@aduth
Copy link
Member Author

aduth commented Feb 5, 2019

There is still dead code left to be eliminated.

I may want to specifically revisit whether to remove the speak option, since there's some commitment for backwards-compatibility there.

@gziolo gziolo removed this from the 5.2 (Gutenberg) milestone Mar 4, 2019
@gziolo gziolo added this to the 5.3 (Gutenberg) milestone Mar 4, 2019
@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019
Base automatically changed from master to trunk March 1, 2021 15:42
@aduth
Copy link
Member Author

aduth commented May 27, 2022

There is still dead code left to be eliminated.

Not sure about the status of dead code these days, but the branch is long-stale, so I doubt it's much use in its current form, so will go ahead and close.

@aduth aduth closed this May 27, 2022
@aduth aduth deleted the revert/legacy-notices-compat branch May 27, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Package] Notices /packages/notices
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with admin_notice
5 participants