-
Notifications
You must be signed in to change notification settings - Fork 11
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
Coding standards, database standards, and performance pass #15
Open
qadan
wants to merge
81
commits into
fsulib:8.x-1.x
Choose a base branch
from
discoverygarden:8.x-1.x
base: 8.x-1.x
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
column rename
Node entities
Null check
update readme, fix issues
first pass at coding standards overhaul
This was referenced Aug 10, 2020
This can happen when switching something from scheduled to indefinite without manually clearing the expiration date before toggling the indefinite radio button in the form. Wasn't breaking anything, but it made the embargoes page look misleading.
…exempt users before this change, if viewing an embargoed object that didn't have any exempt users, then the foreach loop wouldn't be run for the empty array and the message wouldn't be added.
8.x ir 121
update embargo admin permission
…rray empty nids array is returned if no relationships
@@ -22,15 +24,14 @@ function embargoes_schema() { | |||
'not null' => TRUE, | |||
], | |||
'action' => [ | |||
'type' => 'varchar', |
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.
@qadan , I believe the reason why a string was used here was so that a username would persist even after the user is deleted from the Drupal instance.
D9 compat.
Adding a default message for embargo notification block
``` Drupal\Core\Config\Schema\SchemaIncompleteException: Entity type 'Drupal\embargoes\Entity\EmbargoesEmbargoEntity' is missing 'config_export' definition in its annotation ```
Fix for missing entity configuration in Drupal 9
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Supersedes #12, #13, and #14, which I'm closing.
This pull encompasses a great deal of things, so a summary will probably be extremely helpful here:
phpcs
passes forGeneric
,Drupal
, andDrupalPractice
phpcpd
passeslint
passes\Drupal::
has been almost completely removed, except where absolutely necessaryMarkup::create()
orrender()
has been replaced with full renderable arraysembargoes-file-embargoes.js
and its corresponding library file has been removed so that jQuery isn't overriding properties set by the themeEmbargoedAccessInterface
has been implemented to allow entities of various kinds to determine how they wish to restrict access to entity types or provide redirectionnode
,media
, andfile
, that move out a lot of the logic fromembargoes.module
EmbargoesFileAccessHandler
, has been implemented, and asserts itself by altering thefile
entity typeEmbargoesEmbargoesService::getParentNidsOfFileEntity()
now checks references for files inimage
fieldsEventSubscriber
hooking into the request event itself, as the previous method of manually generating aRedirectResponse
and callingsend()
was causing problems with cached headers being present.embargoes.settings
embargoes_log_views
.The one thing that's not implemented here that I very much would have liked is to move the access control stuff to a plugin system. I'm not a huge fan of the fact that I've implemented access control as services, or hard-coded those services into specific hooks/the IP redirect subscriber. This was kind of a quick n' dirty solution; it would make far more sense to implement a plugin so that individual entity types could determine whether or not they should be accessible based on an embargo, then have the access hook and event subscriber vaccum them all up when Drupal builds its cache. That's an improvement for a later day, though.
One current unresolved issue of note is the fact that the
EmbargoesNotificationsForm
isn't wired up to anything - there's noembargoes.notifications
schema, and it doesn't appear thatembargoes.notifications
is referenced anywhere else. This wasn't really considered an issue for this pass but is probably worth looking into a solution in the future.