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

Allow admin pages #255

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Allow admin pages #255

wants to merge 3 commits into from

Conversation

aristath
Copy link
Contributor

@aristath aristath commented Jul 8, 2020

Possible fix for #254

Copy link

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

The Tests/PluginTerritory/NoAddAdminPagesUnitTest.php file has not been adjusted which is why the tests are failing.

@dingo-d
Copy link
Member

dingo-d commented Jul 29, 2020

I kinda think that Travis is failing due to all the changes on develop branch of WPCS 😬

@pattonwebz
Copy link
Member

We probably want to pin a specific version of phpcs then.

@dingo-d
Copy link
Member

dingo-d commented Jul 29, 2020

It's not because of PHPCS, it's because of WPCS. There is an effort to rewrite tons of it using new awesome utils written by @jrfnl. Not 100% sure why it fails, will probably have to look at it in depth when I have the time.

@jrfnl
Copy link

jrfnl commented Jul 29, 2020

@dingo-d I already checked, some of the utilities from WPCS which have been removed are used in sniffs here, that's why. Also with PHPCSUtils in alpha, minimum stability is an issue.

As for updating the Travis script - either fix it to WPCS dev-master instead of dev-develop as "high" version, or considering that there's not that much activity in this repo, leave it for the time being and fix it by upgrading to WPCS 3.0/PHPCSUtils for the next version.

I started updating the Travis script last week and then didn't pull it with the above in mind, but if you like I can pull it anyway.

@dingo-d
Copy link
Member

dingo-d commented Jul 29, 2020

I mean if it solves the current problem then you can update Travis script. But this is actually an excellent opportunity to start using utils here 🙂

@jrfnl
Copy link

jrfnl commented Jul 29, 2020

I mean if it solves the current problem then you can update Travis script. But this is actually an excellent opportunity to start using utils here

@dingo-d Those two things are unrelated.

We can start using PHPCSUtils already, but that wouldn't automatically solve the build failure for WPCS dev-develop as sniffs have been removed etc

So, upgrading to WPCS 3.0.0 and starting to use PHPCSUtils are two different "projects", though they can be done at the same time, in which case it will be easiest to do so once WPCS 3.0.0 is in RC and it is clear what has changed.

Considering there is still plenty to be done for WPCS and PHPCompatibility at the moment, I'm not keen to take on a third such project concurrently, but feel free to get started yourself ;-)

@dingo-d
Copy link
Member

dingo-d commented Jul 29, 2020

Oh I was thinking of doing this myself (a separate task ofc) ^^

@dingo-d
Copy link
Member

dingo-d commented Feb 20, 2021

@aristath We have moved away from Travis to GH actions for CI checks. Can you rebase your PR so that we may see if the checks will pass?

@aristath
Copy link
Contributor Author

Done 👍

@dingo-d
Copy link
Member

dingo-d commented Feb 22, 2021

All the tests are passing so I think it's ok to merge.

@dingo-d dingo-d requested a review from jrfnl February 22, 2021 10:23
Copy link

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Aside from the sniff class docblock still needing to be updated, this largely seems to address #254.

Two concerns:

  1. From the discussion in Allow add_menu_page & add_submenu_page #254, I get the (possibly incorrect) impression that theme admin page additions should be limited to max one top-level item, but that multiple sub-level items are allowed.
    This PR does not enforce that.
  2. From the discussion in Allow add_menu_page & add_submenu_page #254, I get the impression that there was no consensus about the issue.
    Not sure whether that has been resolved in the mean time. Might be a good idea to document in the issue if and when this was discussed again (in Slack or elsewhere) and whether there was consensus in any later discussions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants