- Issue created by @Gábor Hojtsy
- 🇭🇺Hungary Gábor Hojtsy Hungary
This is how it looks like (with Gin) on the backend. The update function worked in my manual testing. The checkbox also worked, it saved properly both checked and unchecked.
- 🇭🇺Hungary Gábor Hojtsy Hungary
Ugh, wrong screenshot :D Here is the correct one.
- 🇭🇺Hungary Gábor Hojtsy Hungary
Since the update function and UI all worked in my manual testing, this should be needs review :)
- 🇭🇺Hungary Gábor Hojtsy Hungary
Hm PHPUnit fail seems to be on this second line, which is odd since there is a lot of things passed earlier:
$this->drupalGet('api/' . $this->branchInfo['project'] . '/duplicates.php/function/duplicate_function'); $this->assertLinkUrl('foo_function', 'http://example.com/function/foo_function', 'foo_function is linked', 'foo_function link goes to right place');
That said, it also triggers this deprecation:
1) /builds/issue/api-3524717/web/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:52 Renderer::renderPlain() is deprecated in drupal:10.3.0 and is removed from drupal:12.0.0. Instead, you should use ::renderInIsolation(). See https://www.drupal.org/node/3407994
- 🇪🇸Spain fjgarlin
Left feedback in the MR. We should display the "Supported" status in the front-end via the templates that we set in this module.
Happy to show the "Supported" or "Unsupported", whatever makes more sense.
- 🇪🇸Spain fjgarlin
We can also add a test for this. I know some of them are failing in HEAD but most are passing so we can create a new one that proves that it works as expected.
- 🇭🇺Hungary Gábor Hojtsy Hungary
Started adding implementation for the sake of discussion. Looks like the implementation would be slightly different based on which page template it is on. Let's agree on a general direction on some questions:
- Should we have a reassuring message when reading current doc? That may be useful, maybe superflous?
- Should we have the logic for displaying messages in twig or in the formatter?
- Is this kind of approach to do a per page type level direction is good or is there a good way to abstract this? (There will be lots of copies of this message otherwise, maybe it should be part of the navigation method/logic, which is added on all pages?).
- Is this markup what we want to use to wrap this message? Would this show fine on api.drupal.org (it looks quite bad on Olivero, but it should pop on api.drupal.org :D).
For comparison this is the visual treatment that Symfony uses inform users:
- 🇪🇸Spain fjgarlin
> Should we have a reassuring message when reading current doc? That may be useful, maybe superfluous?
Happy either way. For simplicity, I would add it, but I'm happy as well if nothing is shown (no-news-is-good-news type of thing)> Should we have the logic for displaying messages in twig or in the formatted?
If we do it in twig, let's make a re-usable template that receives the branch. It could also be done via preprocess functions or in the places where we prepare the variables for the templates. As the logic is not complex, I don't see a problem with it being in twig.> Is this kind of approach to do a per page type level direction is good or is there a good way to abstract this? (There will be lots of copies of this message otherwise, maybe it should be part of the navigation method/logic, which is added on all pages?).
Good point indeed. Maybe we can make it a block plugin. See the existing ones, as we have a method that would help$this->utilities->getProjectAndBranchFromRoute()
. This would avoid duplication in templates.> Is this markup what we want to use to wrap this message? Would this show fine on api.drupal.org (it looks quite bad on Olivero, but it should pop on api.drupal.org :D).
We can have a follow-up styling issue for bluecheese. But we can also use the "info", "warning", "error" messages styling. Don't worry too much about how it looks in Olivero as long as it's right. - 🇭🇺Hungary Gábor Hojtsy Hungary
Video of current MR state :) Still does not work on standalone pages, because there is no classic version nav there.
- 🇭🇺Hungary Gábor Hojtsy Hungary
Added messages to alternate handling too. Still need to be cleaned up and figure out how to do canonical propagation to render array from here.
- 🇭🇺Hungary Gábor Hojtsy Hungary
Ok now this may be the complete version? :)
A. Now displays on all the pages because I made the alternate rendering run even if there were no alternates (will return an empty dataset then for alternates but the canonical/message computation can still run for that as needed).
B Made the messages more friendly like Symfony, eg. adding project name and version number.
C. Added canonical setting for alternate rendering but only if we specifically know which is the alternate version, not in the general situation.Now displays these 3 types of messages:
1. When there was no specific alternate version (note the words are slightly different from the other two). There is no canonical link added in this case to the HEAD.
2. When there was an up to date version but the currently viewed version is still supported. There is a canonical link added in this case to HEAD.
3. When there was an up to date version and the currently viewed version is not supported anymore. There is also a canonical link added in this case to HEAD.
- 🇪🇸Spain fjgarlin
Things are looking really good. I think all we need now is a quick test.
Some handy test helpers can be useful for this case (see usage in other tests):
// Inside the setUp method $this->branchInfo = $this->setUpBranchUi(NULL, TRUE, ['supported' => FALSE]);
You might want to add the default value in "setUpBranchUi" for "supported" as well, and just override when desired.
It's probably a good idea to set up three branches, 1 supported and preferred, 1 supported and 1 not-supported.Then, inside the test, it might be as easy as:
1. Visiting the page of the non-supported branch and checking for the error message.
2. Visiting the page of the supported but non-preferred branch and checking for the warning message.
3. Visiting the page of the supported and preferred branch and checking that there is no error message. - 🇪🇸Spain fjgarlin
I added a test that proves that all three options work.
We'll need to see how this looks in bluecheese, but I think it's good to go. RTBC.
-
fjgarlin →
committed 13629cdd on 2.x authored by
gábor hojtsy →
Issue #3524717 by gábor hojtsy, fjgarlin: Add 'supported' flag to...
-
fjgarlin →
committed 13629cdd on 2.x authored by
gábor hojtsy →
- 🇪🇸Spain fjgarlin
Merged! I will deploy the changes to api.drupal.org this week.
Thanks!