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

Expose quill parser format and blot classes #9860

Closed
wants to merge 4 commits into from

Conversation

bleistivt
Copy link
Contributor

I hit a roadblock, trying to modify a quill format like this:

public function container_init(\Garden\Container\Container $dic) {
    $dic
        ->rule(\Vanilla\Formatting\Quill\Parser::class)
        ->addCall('addFormat', [\Bleistivt\Formats\ImprovedLink::class]);
}

This only lets me add a format to the end of the chain, which will then be wrapped in the old format I am actually trying to overwrite.
(I currently have to use reflection to do this instead.)

For real extensibility (e.g. setting the precedence as well), formatClasses and blotClasses would have to be exposed.

@charrondev
Copy link
Contributor

charrondev commented Dec 19, 2019

@bleistivt I support this, but probably in a more limited way.

I don't think we want to open up wholesale replacing all formats, because it will result in fragile implementations as we add additional ones.

I'd prefer to some implementation like the following:

public function replaceFormat(string $newFormatClass, string $existingFormatClass) {
   // Ensure the new format is subclass the old one. Otherwise throw an error.

   // Replace our existing class definition and only that one if it exists.
}

public function removeFormat(string $existingFormatClass) {
   // Replace our existing format class if it exists.
}

I don't like the wholesale replacement access, because at some point in the near future we might have different sets of formats for different parts of the site, or for different permission levels.

@codecov
Copy link

codecov bot commented Mar 6, 2020

Codecov Report

Merging #9860 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #9860      +/-   ##
============================================
- Coverage     51.53%   51.52%   -0.01%     
  Complexity     7907     7907              
============================================
  Files           357      357              
  Lines         43383    43391       +8     
============================================
  Hits          22357    22357              
- Misses        21026    21034       +8
Impacted Files Coverage Δ Complexity Δ
library/Vanilla/Formatting/Quill/Parser.php 89.42% <0%> (-7.46%) 0 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ed2a79...7c1acf8. Read the comment docs.

@bleistivt
Copy link
Contributor Author

I implemented your suggestion for both format and blot classes.

Is a subclass check really necessary? The class would have to be instantiated for this as far as I understand:
https://www.php.net/manual/en/function.is-subclass-of.php

@charrondev
Copy link
Contributor

charrondev commented Mar 6, 2020

@bleistivt From the PHP manual

Parameters ¶

object
A class name or an object instance. No error is generated if the class does not exist.

class_name
The class name

allow_string
If this parameter set to false, string class name as object is not allowed.
This also prevents from calling autoloader if the class doesn't exist.

There is a lot of specific logic in the blots that checks if things are a certain blot type. If you don't do the subclass check, then these would need to be identified and updated to go find the class instance that is registered here, instead of just using them directly.

@bleistivt
Copy link
Contributor Author

Thanks, I must've missed that.

@stale
Copy link

stale bot commented Mar 19, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: Stale For stale issues that will be automatically closed. label Mar 19, 2021
@bleistivt
Copy link
Contributor Author

A use case for this recently came up here: https://open.vanillaforums.com/discussion/comment/262655#Comment_262655
I'm still closing this, because just as Vanilla doesn't seem to be interested in this, I'm not interested in nannying these pull requests so they're not swallowed by the stale bot.

@bleistivt bleistivt closed this Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Stale For stale issues that will be automatically closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants