-
Notifications
You must be signed in to change notification settings - Fork 37
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
base: develop
Are you sure you want to change the base?
Allow admin pages #255
Conversation
There was a problem hiding this 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.
I kinda think that Travis is failing due to all the changes on develop branch of WPCS 😬 |
We probably want to pin a specific version of phpcs then. |
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. |
@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 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. |
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 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 ;-) |
Oh I was thinking of doing this myself (a separate task ofc) ^^ |
@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? |
Done 👍 |
All the tests are passing so I think it's ok to merge. |
There was a problem hiding this 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:
- 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. - 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.
Possible fix for #254