Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#88 | 1883784-88.patch | 41.6 KB | valthebald |
#88 | interdiff-1883784-86-88.txt | 129 bytes | valthebald |
#86 | interdiff.txt | 2.08 KB | Lal_ |
#86 | 1883784-86.patch | 41.58 KB | Lal_ |
#83 | interdiff.txt | 77.57 KB | Lal_ |
Comments
Comment #1
rfayYou GO! Added @marvil07 to maintainers with full privs...
Comment #2
setvik CreditAttribution: setvik commentedLooking forward to this. In fact; I would love to help out if I can.
Comment #3
marvil07 CreditAttribution: marvil07 commented@setvik: You are very welcome to help. I do not yet have any code for this, but any patches are welcome.
The change notice should be a good start for ideas.
Comment #4
Mile23Pretty please. :-)
Comment #5
stkrzysiak CreditAttribution: stkrzysiak commentedGoing to try to work on this during DrupalCon Pragu Lab Session on Wednesday,
Comment #6
Mile23Feel free to assign yourself back if you're still working on it.
Comment #7
tlyngej CreditAttribution: tlyngej at Annertech commentedI'll try to give this a go.
Just tested out the REST module, so I'll try to throw up what I've got while it's still fresh in my memory. Just to have a starting point.
Comment #8
tlyngej CreditAttribution: tlyngej at Annertech commentedOK, here is my first go.
A lot of commenting is still missing, but it should be a good leg up for some one who might want to do the more educational stuff.
I developed it in beta4.
Comment #9
tlyngej CreditAttribution: tlyngej at Annertech commentedGah, wrong patch.
We'll try this one instead...
Comment #10
tlyngej CreditAttribution: tlyngej at Annertech commentedHmm, better get this re-written for beta6. I see a few changes in the ConfigFormBase().
Comment #12
Mile23Thanks for the work, @tlyngej!
The patch applies (with some whitespace warnings), but I can't install the module. Here's what the log says:
Aside from that, a few more formal things:
For these YML files I'd really like to see some # comments as to what they're for, why they exist, what naming conventions are in use, why different routes were created... Generally make them more readable.
rest_example.module really needs a @defgroup. See the examples module checklist: #2209627: [meta] Module Checklist for Examples We also need a reasonable explanation of what this module does, such as whether it provides a RESTful API, whether it consumes external APIs, etc.
We also need a page that the user can read to find out what's going on. So if they go to /examples/rest_example they see some text explaining what the module does, how to use it, pointers about where they should go and look next within the code, etc.
Comment #13
tlyngej CreditAttribution: tlyngej at Annertech commentedThanks for some really great feedback. I don't see that to often :-)
The problem about installing comes from some changes made to the configuration system, made post beta4, and I will look into that when ever I get a spare moment.
Comment #14
tlyngej CreditAttribution: tlyngej at Annertech commented@Mile23
Got the issue fixed, but I have some issues with getting the authentication, when using ClientInterface::get() method.
Works with other methods, like post, but only for anonymous users when using get.
Comment #15
Mile23So what, specifically, are we trying to demo here? Is it adding a resource to our site? Or is it consuming other APIs?
Comment #16
tlyngej CreditAttribution: tlyngej at Annertech commentedIt's a demo of how Drupal works as a REST server, and how to control entities using \GuzzleHttp\ClientInterface(). This example uses nodes as they works out of the box.
One site can be set as both client and server, or one can make two installations of the example module, and the "client" will be able to manage nodes on the "server" site. Lets pretend that the user has two installations.
The server part of the module comes with a View, that the client can request, that sends back (as JSON) a list of all nodes, of a specific type. From the response the client create a table, containing information about the node, including an edit and delete link (see image attachment Selection_006.png). The client can edit these nodes or delete them. The client can also create new nodes on the remote Drupal site.
I've also created a simple form, that allows one to set the remote site URL, along with username and password for authentication (see image attachment Selection_007.png).
I don't think that consuming API's outside of the Drupal universe would suite this module. That sounds more like a Guzzle/ClientInterface example module to me.
Was that specific enough?
Comment #17
tlyngej CreditAttribution: tlyngej at Annertech commentedShoot, thought I had it.
Turned out that there was a few more API changes in the beta10.
But I think that I'll have something ready soon. Hopefully today.
Comment #18
tlyngej CreditAttribution: tlyngej at Annertech commentedRight, another patch, that seems to be compatible with the beta10 release.
Fingers crossed!
Comment #19
tlyngej CreditAttribution: tlyngej at Annertech commentedComment #20
tlyngej CreditAttribution: tlyngej at Annertech commentedHere is a patch that works with the latest git version of Drupal8.
The String::checkPlain() has changed to SafeMarkup::checkPlain().
Comment #22
tlyngej CreditAttribution: tlyngej at Annertech commentedRe-running the test to make sure everything is still working with the latest code Core.
Comment #24
tlyngej CreditAttribution: tlyngej at Annertech commentedLovely.
Let's re-write that patch then...
Comment #25
Mile23If you check the 'view' link in the test results box, you'll see a list of the failing tests. In this case, they were unrelated to your patch.
I just fixed those today. :-)
So I'm restarting your test.
Comment #27
Mile23Looks like a good start, but there are a few things missing... :-)
First, check out the checklist for examples modules: #2209627: [meta] Module Checklist for Examples
This module doesn't have any tests at all. It needs, at minimum, to test for error-free 200s for all the menus it registers.
For instance, I installed it and went to
examples/rest_client_actions
, but I got an 'unexpected error' message. There needs to be a test that verifies such things don't happen.Some specific things:
It's nice that you're doing due diligence on the input, but that's what FormInterface::validateForm() is for. :-)
I think this needs to change in the other forms as well.
We want a
use
statement with the FQDN and then just reference the easy class name in the rest of the file. So in this case:There are a bunch of these throughout the module.
Comment #28
tlyngej CreditAttribution: tlyngej at Annertech commentedThanks for the review Mile23.
A few questions:
1) What do you think of the level of documentation? This is my first contribution to the example module (as far as I remember) and I'm not sure what would be a reasonable.
2) With "We want a use statement", who is "we" (this modules maintainers? Drupal community? Is there a general guideline?) and why do we not want the FQDN? Normally, when I use FQDN, it's because I only access it once, so creating a
use
statement would only add more code.Comment #29
tlyngej CreditAttribution: tlyngej at Annertech commentedOhh, and at https://www.drupal.org/node/2209627 you mention a top level README. Is this for the content that currently reside in my .module file? None of the other modules has this, so I couldn't just do what ever they do :-)
Comment #30
tlyngej CreditAttribution: tlyngej commentedHere's a new version of the module.
I created use statements and are now using aliases instead.
I created a test class to test the declared menus.
I created
FormInterface::validateForm()
in my form classes and performs the test of missing variables there. Though, I did leave the test for a value in the create/update form, as I need it when I build the form.But I did have some issues though that I was hoping that I could get some help with.
1) I could not run my test, as Drupal failed on a View configuration file (views.view.rest_service). The View is exported through the config interface, and I couldn't figure out what went wrong in that process.
2) I wanted to create a unit test for the
RestExampleClientCalls
class, but I couldn't figure out how to do it with prerequisites the the system has (I need to know the username, password and URL, from the setting form) so I skipped that for now.Comment #31
tlyngej CreditAttribution: tlyngej commentedLets try with a file that contains something this time :-)
Comment #32
tlyngej CreditAttribution: tlyngej commentedUsed an isset() on the result of a function call.
Fixed it in this patch.
Comment #33
Mile23The Royal "We."
Kidding! It's a coding standard: https://www.drupal.org/node/1353118
You can set up various IDEs to help you identify coding standards errors, usually involving the
php_cs
tool: https://www.drupal.org/node/147789If you use NetBeans, then you might want this: https://www.drupal.org/sandbox/mile23/2197899
Also, setting to Needs Review so the testbot can do it's thing. Feel free to do this whenever you submit a patch.
Comment #35
tlyngej CreditAttribution: tlyngej commentedRight there in the first bullet point: "must not use their fully-qualified name inside the code". Don't know why I missed that.
I've been using phpcs with NetBeans for years, but not actually not for D8 projects, as the standard available was VERY broken a while back. Not even Drupal 8 Core was capable of following it. But maybe I should try turning it on again, see if it has gotten better.
Yeah, I forgot the set the issue to "Need review". I was in eager to get to bed, so I forgot :-)
As expected, the test failed because of the View, that I exported from the Configuration Manager, has some discrepancies with the
ConfigSchemaChecker
. I'll try to get someone on IRC to help me out with that.Comment #36
martin107 CreditAttribution: martin107 commentedJust following along ..this is going to be a much needed example topic.
so thanks for working on it. :)
Just picking up a few coding standard errors. nothing substantial.
Comment #38
tlyngej CreditAttribution: tlyngej commentedExcelent Martin. Thanks a lot for the changes.
Comment #39
martin107 CreditAttribution: martin107 commentedI'm just looking at RestExampleClientCalls in this change.
1) I want to harmonize the return value of the family of calls
RestExampleClientCalls::create()/update()/delete()
If the configuration setting rest_example.settings:server_url is not defined.
then RestExampleClientCalls::remoteUrl is undefined.
Uncomfortably before this patch the return type of create/update/delete changed to void/null from Response object
Instead I have change this to a 500 Response object saying in effect "I haven't been configured yet."
I've not found the right message text yet, so I've gone with something simple
"The remote URL has not been setup."
2) Assigning a constant to RestExampleClientCalls::clientHeaders can be done outside the constructor in a slightly simpler way.
3) I am now injecting the config_factory clearing the hidden dependancy on Drupal.php.
4) The class now passes a coding standard review as defined by phpcs --standard=Drupal
So I ve added lots of annotation, camel cased state variables, and fixed some indentation issues.
Comment #43
tlyngej CreditAttribution: tlyngej at Annertech commentedUsing the corrections from #39 I've created a new patch.
Hopefully the exported View in this can pass the test. :-S
Comment #44
tlyngej CreditAttribution: tlyngej at Annertech commentedComment #46
tlyngej CreditAttribution: tlyngej at Annertech commentedTrying a new patch that fixes the errors the CI complained about.
Comment #48
Mile23It looks like there's a branch blocker. Don't sweat the fails that are unrelated to your module.
Comment #49
martin107 CreditAttribution: martin107 commentedDigging through the test fails I see this
Declaration of Drupal\field_example\Plugin\Field\FieldFormatter\SimpleTextFormatter::viewElements() must be compatible with Drupal\Core\Field\FormatterInterface::viewElements(Drupal\Core\Field\FieldItemListInterface $items, $langcode)
this is the change record we must use to fix the problem
https://www.drupal.org/node/2575729
in short, we need to add an extra $langcode parameter to SimpleTextFormatter::viewElements()
I would do this myself ... but I am busy today sorry.
Comment #50
Mile23Yah, I'm working on it. This isn't the place discuss, however. :-)
Comment #52
jerryimiolo CreditAttribution: jerryimiolo as a volunteer commentedWhite space errors but still applied. Couldn't install because of php error. Error below ...
[Sat Jan 30 11:43:54.605647 2016] [fcgid:warn] [pid 18064:tid 2973798400] [client 127.0.0.1:53291] mod_fcgid: stderr: Uncaught PHP Exception Drupal\\Core\\Config\\UnsupportedDataTypeConfigException: "Invalid data type in config views.view.rest_service: A YAML file cannot contain tabs as indentation at line 30 (near "\ttype: perm")." at /Users/imioloj/Downloads/drupal/core/lib/Drupal/Core/Config/FileStorage.php line 103, referer: http://drupal.dd:8083/admin/modules/list/confirm
Comment #53
tlyngej CreditAttribution: tlyngej at Annertech commented@jerryimiolo I just tested with Drupal 8.0.3 on my own machine and on simplytest.me with version 8.0.4-dev. Installation went fine bot times.
What version of Drupal are you using?
Comment #54
Mile23Some review:
It needs to modify examples.module so it has a toolbar link to whichever is the 'main' page.
There should be more explanation on the pages as to what to do and how to recover from a problem.
If you don't set a remote host and then try to create a node, there is no error even though no node is created.
I tried to set my local host as the remote and then create a node, and I got this error:
Uncaught PHP Exception GuzzleHttp\Exception\ClientException: "Client error: 405" at /Users/paul/pj2/drupal/vendor/guzzlehttp/guzzle/src/Middleware.php line 69
Note that this is the same Drupal instance as client and server, and I don't know if that matters, but it seems that'd be an obvious use case for an example.Also there are some @todos in the code that might be stale given the age of this patch.
Comment #55
kay_v CreditAttribution: kay_v as a volunteer commentedI also had no issues with installation just now on simplytest.me, which is using Drupal 8.0.7-dev, fwiw. Not a full review, but ideally this report is of some use for now with more to come from me.
Comment #56
kay_v CreditAttribution: kay_v as a volunteer commentedwhups - links for REST examples both worked on SimplyTest.me instance until I entered client connection info
then when I visit /examples/rest_client_actions I get the following cryptic error "The website encountered an unexpected error. Please try again later."
no issues with the usual suspect links that seem often to throw that error (e.g. the modules page); just /examples/rest_client_actions
Comment #57
tlyngej CreditAttribution: tlyngej at Annertech commentedComment #58
tlyngej CreditAttribution: tlyngej at Annertech commentedAnother version (finally).
Comment #59
tlyngej CreditAttribution: tlyngej at Annertech commentedComment #60
tlyngej CreditAttribution: tlyngej at Annertech commentedI can now see that I wrote this before I knew how services in Drupal worked.
I should re-factor this patch to use dependency injection properly.
Comment #61
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedQuick look on patch:
dependencies should be in format {project_name}:{module}
A pretty new line :)
As you said, DI would be better !
Comment #62
Mile23Comment #63
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedRe-rolling and updating the patch.
Comment #64
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedAdded description template, made RestExampleClientCalls a service and few more adjustments.
Comment #65
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #66
Mile23The examples/rest-client-actions path is giving me this:
The messenger service is 8.5.x+ https://www.drupal.org/node/2774931
We want to target core 8.4.x, since that's the current stable release.
Reviewing:
Settings and actions should be children of help, and help should be
expanded: TRUE
.Help should be titled 'REST Example' and the others should be called 'Client settings' and 'Client actions'.
Should be
_permission: 'access content'
unless there's some reason they need different permissions. And if they need different permissions, then that should be explained in comments. We should use some kind of content admin permission for the routes that can change content.Comment #67
Mile23This content type persists after the module is uninstalled.
So like in node_type_example we should add:
Comment #68
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #69
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #70
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #72
cleaver CreditAttribution: cleaver as a volunteer commentedRerolled the patch.
Comment #73
Mile23Setting to NR to trigger a testbot run.
Comment #75
Lal_Bringing this back.... I think we might need some actual work on this but I am blank on the implementation please review and suggest some changes...
Comment #77
Lal_coding standards
Comment #78
Lal_Comment #79
jungleThe default branch is 3.x-dev now, which is Drupal 9 compatible.
Comment #80
jungleSetting to NW as testing did not pass.
Comment #81
jungleFYI, A VUE.js based REST API example.
Comment #82
valthebaldMoving to new examples meta
Comment #83
Lal_Comment #84
Lal_Comment #86
Lal_Comment #87
valthebaldComment #88
valthebaldReplacing core version with core_version_requirement in info.yml
Comment #90
valthebaldPatch from #86 looks good, and testbot is happy. Thanks everyone for the tremendous effort over 7 years!
Comment #92
jungleBTW, here should be
drupal:views
which failed testing on HEAD. Fixing in #3160118: Fix broken testing on HEAD